[PATCH] D96285: [clang][Arm] Fix handling of -Wa,-implicit-it=

[David Spickett via Phabricator via cfe-commits]

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