[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Overall, this looks ok, but I don't feel like I'm qualified to approve this patch, since I'm not really familiar w/ how this tool is used in practice. Some comments nonetheless:

> Those relative paths are meant to be resolved relative to the corresponding build directory.

Is this behavior documented somewhere?



================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:156
+                        const tooling::TranslationUnitDiagnostics *SourceTU,
+                        const std::string *buildDir) {
     // Use the file manager to deduplicate paths. FileEntries are
----------------
let's use an optional -- when populated, we have to copy it anyhow, so there's nothing gained by passing by pointer, and it comes at the cost of a nullable pointer.


================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
     // automatically canonicalized.
+    auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    auto PrevWorkingDir = WorkingDir;
----------------
Why are you capturing this as a reference? This is a subtle and IMO error prone pattern, since it's not obvious in the code below that you're mutating a deeply nested element of the filesystem.  Can  you instead use a local variable and just set this right before you need it?


================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:162
+    if (buildDir) {
+      WorkingDir = *buildDir;
+    }
----------------
assuming this is now an optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647