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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a subscriber: alexfh.
avogelsgesang added a comment.

>> Those relative paths are meant to be resolved relative to the corresponding build directory.
>
> Is this behavior documented somewhere?

I couldn't find this documented anywhere. My assumption is based on behavior which I observed from clang-tidy. Without this patch, `clang-apply-fixes` failed to apply the changes exported by `clang-tidy --export-fixes` due to its inability to find some source files referred to through relative paths

It seems the `clang::tooling::Diagnostic::BuildDirectory` was introduced in https://reviews.llvm.org/D26137. Maybe @alexfh who reviewed that patch can provide some more context here?



================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
     // automatically canonicalized.
+    auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    auto PrevWorkingDir = WorkingDir;
----------------
ymandel wrote:
> 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?
> Why are you capturing this as a reference?

Mostly to keep the code shorter. Otherwise, I would have to write

```
    auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
    if (buildDir)
      SM.getFileManager().getFileSystemOpts().WorkingDir = *buildDir;
    // [...]
    WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
```

which isn't really DRY.

> Can you instead use a local variable and just set this right before you need it?

I think I don't understand the alternative you are proposing here. Can you provide an example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647