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

[Caroline Tice via Phabricator via cfe-commits]

cmtice marked 3 inline comments as done.
cmtice added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3246-3247
         DWARFVersion = ExplicitVersion;
   }
+  else if (DefaultDWARFVersion != 0)
+    DWARFVersion = DefaultDWARFVersion;
----------------
dblaikie wrote:
> dblaikie wrote:
> > 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))
> Oh, there's also clang/tools/clang-format/git-clang-format for formatting anything in a git revision range.
Ok, will do.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3247-3248
   }
+  else if (DefaultDWARFVersion != 0)
+    DWARFVersion = DefaultDWARFVersion;
 
----------------
dblaikie wrote:
> 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).
This covers the case where you are setting the default dwarf version without actually turning on/off debug info at all (there's no -gdwarf or -g option, so EmitDwarf is false).


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6173-6175
+    unsigned DefaultDwarfVersion = ParseDebugDefaultVersion(getToolChain(), Args);
+    DwarfVersion = DefaultDwarfVersion ? DefaultDwarfVersion
+                   :getToolChain().GetDefaultDwarfVersion();
----------------
dblaikie wrote:
> 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?
Ok will do.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69822