jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.
Basically LGTM - (I'm happy for this to land either with or without my comments
being addressed). Would be nice to get a second reviewer to confirm they're
happy.
================
Comment at: clang/test/Driver/fbinutils-version.c:1
+// RUN: %clang -### -c -target x86_64-linux %s -fbinutils-version=none 2>&1 |
FileCheck %s --check-prefix=NONE
+
----------------
Maybe also add a case for `-fbinutils-version=2` (i.e. no minor version at all).
================
Comment at: llvm/lib/CodeGen/LLVMTargetMachine.cpp:64
+ if (Options.BinutilsVersion.first > 0)
+ TmpAsmInfo->setBinutilsVersion(Options.BinutilsVersion);
----------------
Is it possible somebody might do something weird like `-fbinutils-version=0.1`
and get behaviour different to what might be expected? Should we explicitly
forbid major versions of 0?
================
Comment at: llvm/lib/Target/TargetMachine.cpp:239
+ if (Version == "none")
+ return {INT_MAX, 0}; // Make binutilsIsAtLeast() return true.
+ std::pair<int, int> Ret;
----------------
Maybe `{INT_MAX, INT_MAX}`? Doesn't really matter in practice, but `{INT_MAX,
INT_MAX} > {INT_MAX, 0}` in the versioning scheme.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85474/new/
https://reviews.llvm.org/D85474
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
