[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3246-3247
         DWARFVersion = ExplicitVersion;
   }
+  else if (DefaultDWARFVersion != 0)
+    DWARFVersion = DefaultDWARFVersion;
----------------
Looks like this should be on a single line to conform to LLVM convention (though might just be phabricator doing something weird)

If you can run clang-format over the change (not over the whole file) it should fix up issues like this. (there's various clang-format editor integrations - there's some google-internal documentation at go/clang-format that'll explain how to setup an auto-save hook that'll clang-format the changed lines so all your C++ code in the LLVM repository conforms to LLVM's coding conventions (well, those that can be expressed by clang-format))


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3247-3248
   }
+  else if (DefaultDWARFVersion != 0)
+    DWARFVersion = DefaultDWARFVersion;
 
----------------
Hmm, actually - why is this case necessary/what does it cover? I was hoping the case you added inside the "if (EmitDwarf)" case above would be all that was required (& the call to ParseDebugDefaultVersion would go inside there at the use, to reduce the variable scope/keep the code closer together).


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6173-6175
+    unsigned DefaultDwarfVersion = ParseDebugDefaultVersion(getToolChain(), Args);
+    DwarfVersion = DefaultDwarfVersion ? DefaultDwarfVersion
+                   :getToolChain().GetDefaultDwarfVersion();
----------------
Some clang-formatting required, but also could probably be written as:

  if (DwarfVersion == 0)
    DwarfVersion = ParseDebugDefaultVersion(...);

  if (DwarfVersion == 0)
    DwarfVersion = getToolChain().GetDefaultDwarfVersion();

To make these more symmetric?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69822/new/

https://reviews.llvm.org/D69822