ymandel added a comment.
Overall, this looks ok, but I don't feel like I'm qualified to approve this
patch, since I'm not really familiar w/ how this tool is used in practice. Some
comments nonetheless:
> Those relative paths are meant to be resolved relative to the corresponding
> build directory.
Is this behavior documented somewhere?
================
Comment at:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:156
+ const tooling::TranslationUnitDiagnostics *SourceTU,
+ const std::string *buildDir) {
// Use the file manager to deduplicate paths. FileEntries are
----------------
let's use an optional -- when populated, we have to copy it anyhow, so there's
nothing gained by passing by pointer, and it comes at the cost of a nullable
pointer.
================
Comment at:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
// automatically canonicalized.
+ auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+ auto PrevWorkingDir = WorkingDir;
----------------
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?
================
Comment at:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:162
+ if (buildDir) {
+ WorkingDir = *buildDir;
+ }
----------------
assuming this is now an optional.
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
