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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Overall, looks good to me. But, I'd like to get a more experienced reviewer to confirm they're comfortable with the new behavior.



================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
     // automatically canonicalized.
+    auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    auto PrevWorkingDir = WorkingDir;
----------------
avogelsgesang wrote:
> 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?
What you wrote is what I had in mind, though the last line, i think, would be:
`SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;`
?

That said, I see your point.  The problem is not in your code so much as in the underlying (lack of) API for setting and restoring the working directory. Honestly, the need to manually restore bothers me more than the reference, now that I consider it in more detail. Perhaps another reviewer will have a better suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647