dexonsmith added inline comments.
Herald added a project: All.
Comment at:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:178
}
+SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
};
This looks incorrec
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8188484daa41: [clang-apply-replacements] Correctly handle
relative paths (authored by avogelsgesang, committed by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://revie
ymandel added a comment.
I can land it for you. should be sometime this afternoon, but ping me tomorrow
if you don't see any activity.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.org/D112647
avogelsgesang added a comment.
Thanks for the reviews, @ymandel and @hokein!
What are the next steps to get this landed?
(I am a first time contributor and have no commit access)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://re
avogelsgesang updated this revision to Diff 386485.
avogelsgesang added a comment.
Don't update WorkingDir through local reference; as requested @ymandel and
@hokein
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.or
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 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().getFil
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 canoni
avogelsgesang added a comment.
@alexfh, @ymandel: Do you know who could review this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.org/D112647
___
cfe-commits mail
avogelsgesang updated this revision to Diff 384239.
avogelsgesang marked 2 inline comments as done.
avogelsgesang added a comment.
Use `llvm::Optional` instead of pointer as suggested by @ymandel
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/
avogelsgesang added a subscriber: alexfh.
avogelsgesang added a comment.
>> Those relative paths are meant to be resolved relative to the corresponding
>> build directory.
>
> Is this behavior documented somewhere?
I couldn't find this documented anywhere. My assumption is based on behavior
whi
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 direc
ymandel added a comment.
I'm not really familiar w/ this code. But, if njames93 doesn't respond, then
I'll give it a go.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.org/D112647
__
avogelsgesang added reviewers: ymandel, njames93.
avogelsgesang added a comment.
Adding reviewers based on `git log`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.org/D112647
___
avogelsgesang updated this revision to Diff 382741.
avogelsgesang added a comment.
Remove unrelated changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112647/new/
https://reviews.llvm.org/D112647
Files:
clang-tools-extra/clang-apply-replaceme
avogelsgesang created this revision.
Herald added a subscriber: carlosgalvezp.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
The fixes from the YAML file can refer to relative paths.
Those relative paths are mean
16 matches
Mail list logo
