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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

this change looks good to me, I will leave the final stamp for @ymandel.



================
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:
> 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.
rather than keeping the code short, I would be more explicit on setting and restoring the working directory, it is easier for readers to understand the intention (the FileManager's getter API which returns a mutable reference seems a bit weird to me).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647