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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
