[PATCH] D73383: Allow retrieving source files relative to the compilation directory.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1085
   StringRef FileName = *Name;
-  if (Kind != FileLineInfoKind::AbsoluteFilePath ||
+  if (Kind == FileLineInfoKind::None || Kind == FileLineInfoKind::Default ||
       isPathAbsoluteOnWindowsOrPosix(FileName)) {
----------------
The first half of this check is dead: there's a return false earlier for `Kind == FileLineInfoKind::None`

I'd also like to see tests using `None` and `Default` kinds.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:909
+  LineTable &LT = Gen->addLineTable();
+  LT.setCustomPrologue({
+      {33, LineTable::Long},   // unit length
----------------
Rather than needing to create an entire custom prologue, you can use `createBasicPrologue` to generate a default one, and then just tweak the contents to suit your needs (not forgetting to update the length fields in the process). See `ErrorForUnitLengthTooLarge` for an example usage.

This will improve readability as it will more clearly show what is important to to this test.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:949
+  std::string Result;
+  // DWARF 5 stores the comp dir in two places: the Compilation Unit and the
+  // directory table entry 0, and implementations are free to use one or the
----------------
Could we use "compiler directory" in the comment please?


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:952
+  // other. This copy serves as the one stored in the CU.
+  llvm::StringRef CompDir = "/a";
+  EXPECT_TRUE(
----------------
No need to specify `llvm::` here.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:955
+      (*ExpectedLineTable)->Prologue.getFileNameByIndex(
+          0, CompDir, DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath, Result));
+  EXPECT_STREQ(Result.c_str(), "/a/b/c");
----------------
I think you haven't clang-formatted this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73383