DavidSpickett added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2307
+static bool AddImplicitITArgs(const ArgList &Args, ArgStringList &CmdArgs,
+ StringRef Value) {
----------------
I would rename this AddARMImplicitITArgs to match AddARMTargetArgs.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2386
+ if (Value.startswith("-mimplicit-it="))
+ if (AddImplicitITArgs(Args, CmdArgs, Value.split("=").second))
+ continue;
----------------
You could merge the two ifs with &&
================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:13
+/// Test space seperated -Wa,- arguments.
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mthumb
-Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s --check-prefix=ALWAYS
+// RUN: %clang -target arm-linux-gnueabi -### -Wa,-mthumb
-Wa,-mimplicit-it=never %s 2>&1 | FileCheck %s --check-prefix=NEVER
----------------
For these multiple option tests, we're checking that the last -mimplicit-it
wins, so I think the first argument should be another different value. E.g.
-Wa,-mimplicit-it=thumb -Wa,-mimplicit-it=always %s 2>&1 | FileCheck %s
--check-prefix=ALWAYS
Then if we got our logic wrong, we'll see "=thumb" in the result and fail.
(afaik -mthumb doesn't set implicit-it to anything)
================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:26
+// RUN: %clang -target arm-linux-gnueabi -### -Xassembler -mimplicit-it=foo %s
2>&1 | FileCheck %s --check-prefix=XINVALID
+
+
----------------
Does it make sense to check which wins of -mimplicit-it and -Wa,-mimplicit-it
for assembler and c inputs?
I guess right now we would have the first for C files, and both for assembler
files but the last -mllvm implicit-it wins, and it'd be the assembler's option.
Either way some tests to confirm that would be good.
You can use any old C file, my change that you reviewed does the same thing to
check this. (but in that case, since it was earlier it could tell whether we
had both kinds of option which I don't think can be done here)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96285/new/
https://reviews.llvm.org/D96285
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
