ymandel added inline comments.
================
Comment at:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
// automatically canonicalized.
+ auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+ auto PrevWorkingDir = WorkingDir;
----------------
hokein wrote:
> 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).
Ok, let's do that then. It's "wordy" but it will be very clear what's going
on, and seems the best we can do given the odd API.
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
