cpsauer added a comment.
@kadircet, could I get your review on this one?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
cpsauer added a comment.
@kadircet, friendly check in, since I got reminded of this: How would you like
to proceed from here?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
___
cfe-commits mailing
cpsauer added a comment.
Thanks, Nathan. Makes sense; sounds good.
@kadircet, I think this one is ready for you, whenever you want it. Lmk how
you'd like to proceed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
cpsauer updated this revision to Diff 491593.
cpsauer added a comment.
Rebased
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer updated this revision to Diff 491592.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer added a comment.
Yay. Thanks, Nathan, for guiding, replying, and just generally being so kind,
great, and thorough.
I'll back out the plugin part of the changes changes momentarily, then.
Just to confirm: Sounds like you're not concerned, then, that the
JSONCompilationDatabasePlugin
cpsauer added a comment.
(If anyone knows what's going on with the (unrelated seeming?) testing timeouts
please do say.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
___
cfe-commits mailing list
cpsauer updated this revision to Diff 489317.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer updated this revision to Diff 489217.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer updated this revision to Diff 489216.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer updated this revision to Diff 489214.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
cpsauer updated this revision to Diff 489208.
cpsauer added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
@nridge, I took a shot at the change you suggested. Confirming that this what
you were thinking of, removing `inferTargetAndDriverMode` from database load
cpsauer added a comment.
@nridge, yep, confirming: For Android, --target is being added explicitly as
part of the command and we'll need to pass through explicit (but not implicit)
--target flags to the system includes extractor to get the correct paths.
CHANGES SINCE LAST ACTION
cpsauer added a subscriber: nridge.
cpsauer added a comment.
Ooh interesting. Thanks for your careful look, @ArcsinX.
To check my understanding: Sounds like, more generally, commands get massaged
into clang argument form before they're passed into the query-driver machinery,
which can break
cpsauer added a comment.
Sweet. Thanks again, Nathan.
Guys, lmk how you'd like to go from here. If you approve; feel free to copy
in/land as part of the other change if that would save time.
(I don't have commit access anyway.)
CHANGES SINCE LAST ACTION
cpsauer added a comment.
@kadircet, you were asking about what's requiring the use of query-driver--I'll
reply over in the issue (as you suggest), keeping everything in one place.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
cpsauer added a comment.
@nridge, I took a shot at amending the test. Thanks for the pointer! Please me
know if that looks good to you!
The host-target cross-compile situation you hypothesize is exactly what caused
me to notice this. Super common with Bazel.
CHANGES SINCE LAST ACTION
cpsauer updated this revision to Diff 481141.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/SystemIncludeExtractor.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
Index:
cpsauer added a comment.
Sorry for being so slow this time myself, guys. Took me a while to dig myself
out of a deep inbox hole after Thanksgiving (US holiday). Really appreciate you
all and your friendly responsiveness, especially as I get up to speed here.
Repository:
rG LLVM Github
cpsauer added a comment.
I suppose, if it ever might help make the case with said employer (Google,
right?) the broken, non-stock-clang use case that's motivating this is Android
(and also usage from Bazel/Blaze).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
cpsauer added a comment.
Makes sense! Thanks for your time, Sam, and for being great, as always.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
___
cpsauer added a comment.
Sam, my read, too, is that the memoizing design isn't safe--also that the key
bug is preexisting.
I had the same reaction reading through this file after spotting problems in
the log. That's what spawned https://github.com/clangd/clangd/issues/1378.
Any chance I could
cpsauer created this revision.
cpsauer added reviewers: nridge, sammccall, kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
cpsauer requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project:
cpsauer added a comment.
Wahoo! Thank you
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114213/new/
https://reviews.llvm.org/D114213
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
cpsauer added a comment.
@sammccall (or others), could I ask for your help landing the change now that
it's approved?
(I think I need to ask someone with commit access to do so, per
https://llvm.org/docs/Phabricator.html#committing-a-change)
Repository:
rG LLVM Github Monorepo
CHANGES
cpsauer added a comment.
Good call, @nridge. Will do as soon as we land this one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114213/new/
https://reviews.llvm.org/D114213
___
cfe-commits mailing list
cpsauer created this revision.
cpsauer added a reviewer: sammccall.
cpsauer created this object with visibility "All Users".
Herald added subscribers: usaxena95, kadircet.
cpsauer requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project:
27 matches
Mail list logo