This revision was automatically updated to reflect the committed changes.
Closed by commit rG26fa9e31f58a: Add support to find out resource dir and add
it as compilation args (authored by kousikk).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/
Bigcheese added a comment.
Thanks, you're good to commit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.or
kousikk added inline comments.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:75-79
+llvm::Optional Redirects[] = {
+{""}, // Stdin
+StringRef(OutputFile),
+StringRef(ErrorFile),
+};
Bigcheese wrote:
> It would be nice i
kousikk updated this revision to Diff 230508.
kousikk marked 4 inline comments as done.
kousikk added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/
Bigcheese accepted this revision.
Bigcheese added a comment.
This revision is now accepted and ready to land.
lgtm with the changes to `FindResourceDir`.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:50
+ /// compiler path is NOT an absolute path.
+ std::string Fi
kousikk added a comment.
Ping
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi
kousikk updated this revision to Diff 227601.
kousikk added a comment.
Forgot to run clang-format
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
Files:
clang/tools/clang-scan-deps/ClangScanDeps.cpp
Inde
kousikk updated this revision to Diff 227600.
kousikk marked an inline comment as done.
kousikk added a comment.
@BigCheese - thanks, all the three points make sense!
> An alternative would be to only do FindResourceDir when given an absolute
> path to a compiler, instead of trying to guess. I t
Bigcheese requested changes to this revision.
Bigcheese added a comment.
This revision now requires changes to proceed.
So I agree solving this problem makes sense, but I have some issues with the
current patch.
- This defaults to doing the lookup. The test suite, and some real outputs,
just us
kousikk added a comment.
I discussed with @klimek about this. The conclusion we arrived at was:
Allowing override of `resource-dir` for all tools is not something we should do
(for reasons that @klimek has outlined above). However, the dependency scanner
tool can choose to override `resource-di
klimek added a comment.
> since resource directory should be picked relative
> to the path of the clang-compiler in the compilation command.
The resource dir should be the resource dir that shipped with the clang source
code that the *tool* was built with.
We can think about the resource dir as
Bigcheese added a reviewer: klimek.
Bigcheese added a comment.
I've added Manuel as a reviewer as this patch is also changing the tooling APIs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
MaskRay added inline comments.
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:43
ArgumentsAdjuster getClangStripOutputAdjuster() {
- return [](const CommandLineArguments &Args, StringRef /*unused*/) {
+ return [](const CommandLineArguments &Args, StringRef /*unused*/, St
Bigcheese added a comment.
The current assumption is that the clang-scan-deps binary is the one that comes
next to the clang binary you are using. There are lots of other differences
between clang versions than just the resource-dir.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST AC
kousikk added a comment.
> I haven't looked into this in detail but it feels kind of wasteful to start a
> process just to get this value.
Right, that's why I cache this information. So multiple compile commands
sharing the same compiler process will trigger at 1 subprocess and then
subsequent
arphaman added a comment.
@kousikk could we please add support for generic `-extra-arg`, which can then
add `-resource-dir` like the other clang tools?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69122/new/
https://reviews.llvm.org/D69122
___
jkorous added inline comments.
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:70
+};
+if (const int RC = llvm::sys::ExecuteAndWait(ClangBinaryPath,
PrintResourceDirArgs, {}, Redirects)) {
+ auto ErrorBuf = llvm::MemoryBuffer::getFile(ErrorFile.c_str());
-
kousikk created this revision.
kousikk added reviewers: arphaman, Bigcheese, jkorous, dexonsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
kousikk edited the summary of this revision.
kousikk edited the summary of this revision.
If -resource-dir is not specified as pa
18 matches
Mail list logo
