[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.


================
Comment at: lib/Driver/ToolChain.cpp:549-556
+    bool IsIntegratedAssemblerThumb = false;
+    for (const Arg *A :
+         Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+      for (StringRef Value : A->getValues()) {
+        if (Value == "-mthumb")
+          IsIntegratedAssemblerThumb = true;
+      }
----------------
compnerd wrote:
> Why not write this as:
> 
>     const auto &Values = Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler);
>     bool IsIntegratedAssemblerThumb =
>         std::any_of(std::begin(A->getValues()),
>                     std::end(A->getValues()),
>                     [](StringRef Arg) { return Arg == "-mthumb"});
I gave it a try and I don't think it can be done simply as there are two loops and not one. There could be two Args, one for Wa_COMMA and one for X_Assembler. Unless there is a way of combining the iterator ranges into one I don't think this can be done without nested loops. I checked the other uses of Args.filtered and I couldn't find any of use outside of the range for. Please do let me know if I'm missing something though as I'm happy to change it if there is something better. 


================
Comment at: lib/Driver/ToolChain.cpp:560-564
+    if ((InputType != types::TY_PP_Asm &&
+         Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+                      ThumbDefault)) ||
+        (InputType == types::TY_PP_Asm && IsIntegratedAssemblerThumb) ||
+        IsMProfile || getTriple().isOSWindows()) {
----------------
compnerd wrote:
> This is horribly complicated to read.  Can we just split this out of the condition and create a variable?
Agreed. I'll post an alternative that attempts to evaluate -mthumb before the boolean expression. 


https://reviews.llvm.org/D40127