[PATCH] D82974: [DebugInfo] Allow GNU macro extension to be read

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3055-3059
+    auto FormToString = [](unsigned Form) {
+      return dwarf::MacroString(Form, /*GNU=*/false);
+    };
     emitMacroFileImpl(F, U, dwarf::DW_MACRO_start_file,
+                      dwarf::DW_MACRO_end_file, FormToString);
----------------
dblaikie wrote:
> I'd probably inline the lambda into the call expression - it's short/simple enough.
This change is now removed. I'll inline the expression in the follow-up patch.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:535
+                    unsigned EndFile,
+                    std::function<StringRef(unsigned Form)> MacroFormToString);
   void handleMacroNodes(DIMacroNodeArray Nodes, DwarfCompileUnit &U);
----------------
dblaikie wrote:
> Probably llvm::function_ref here, since the functor doesn't escape the callee
Yes, thanks! Since I moved out the GNU defines to a separate GnuMacroString() function this change is done in the patch which introduces emission of the extension.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:57
+        WithColor(OS, HighlightColor::Macro).get()
+            << MacroString(E.Type, /*GNU=*/Macros.Header.Version < 5);
       else
----------------
ikudrin wrote:
> If this is written like
> ```
> << (Macros.Header.Version < 5 ? GnuMacroString(E.type) : MacroString(E.type)) ;
> ```
> 
> then `llvm::dwarf::MacroString()` can be left as is and most of the changes are not needed.
Yes, thanks! Since there are so few uses where we conditionally want to select either the DWARF or GNU defines that seems cleaner.


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

https://reviews.llvm.org/D82974