[PATCH] D42765: [DebugInfo] Support DWARFv5 source code embedding extension

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: docs/AMDGPUUsage.rst:858
+Source text for online-compiled programs (e.g. those compiled by the OpenCL
+runtime) may be embedded into the DWARFv5 line table using the ``clang
+-gembed-source`` option, described in table :ref:`amdgpu-debug-options`.
----------------
I learned yesterday that we stylize this as `DWARF v5`


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:40
     MD5::MD5Result Checksum;
+    bool HasSource = false;
+    StringRef Source;
----------------
Alignment-wise it might be better to swap these two. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:76
     bool HasMD5;
+    bool HasSource;
     std::vector<uint8_t> StandardOpcodeLengths;
----------------
Maybe add a comment like the one above. Now these two look grouped. 


================
Comment at: include/llvm/IR/DIBuilder.h:151
+                       StringRef Checksum = StringRef(),
+                       Optional<StringRef> Source = None);
 
----------------
I'm not a fan of these two things not being consistent. Either both should be options or both should be empty StringRefs? This seems to be recurring throughout this patch. I think `Optional` better conveys the idea, but would require more API changes. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:923
+    return Entry.Source;
+  else
+    return None;
----------------
`else` can be omitted here. 


================
Comment at: lib/MC/MCParser/AsmParser.cpp:3363
+        return true;
+    } else {
+      return TokError("unexpected token in '.file' directive");
----------------
`else` can be omitted here too. 


Repository:
  rL LLVM

https://reviews.llvm.org/D42765