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