[PATCH] D95753: Store compilation dir separately in coverage mapping

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

This looks great, that turned out to be a lot cleaner than I expected.



================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1717
+  FilenameStrs[0] = getCurrentDirname();
+  FilenameRefs[0] = FilenameStrs[0];
   for (const auto &Entry : FileEntries) {
----------------
There's some nice alignment here with your change that gets rid of DecompressedData. If CoverageFilenamesSectionWriter were to accept an ArrayRef<string>, this strange FilenameRefs container can go away.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5302
+  // Add in -fcoverage-compilation-dir if necessary.
+  if (!Triple.isNVPTX() && !Triple.isAMDGCN())
+    addCoverageCompDirArg(Args, CmdArgs, D.getVFS());
----------------
Perhaps leave a comment explaining why the compdir flag differs for these GPU targets?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h:178
 private:
-  std::vector<StringRef> Filenames;
+  std::vector<std::string> Filenames;
   std::vector<ProfileMappingRecord> MappingRecords;
----------------
This is a nice cleanup that should probably happen before/independently of the format change.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:149
+  StringRef CWD;
+  if (auto Err = readString(CWD))
+    return Err;
----------------
This would need to be gated on the CovMapVersion. For context, the format needs to maintain backwards compatibility so that newer IDE's can inspect binaries built by older compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95753