klimek added inline comments.
================
Comment at: clangd/IncludeScanner.cpp:75
+ bool WasEmpty = Queue.empty();
+ for (const auto &Cmd : Cmds) {
+ QueueEntry E(Cmd, VFS);
----------------
Usually I'd try to not lock a loop, as a large number of compile commands now
blocks other threads for a bit. Fairness might not matter here, though.
================
Comment at: clangd/IncludeScanner.cpp:97
+ auto &Entry = Queue.front();
+ Lock.unlock();
+ process(std::move(Entry));
----------------
I'd find it a bit more idiomatic to have two unique_locked blocks instead
(easier to pull out a function when things grow etc), but I see it's annoying
to have to default-construct an entry in that case :(
Given that I'd probably pull that into a function, but then we need to
communicate Done. Sigh.
================
Comment at: clangd/IncludeScanner.cpp:100
+ Lock.lock();
+ Queue.pop_front();
+ if (Queue.empty()) {
----------------
I'd try to write the code so multiple consumer threads would work - given that
process() can't fail, why not pop in the locked session above?
================
Comment at: clangd/IncludeScanner.cpp:131
+ IgnoringDiagConsumer IgnoreDiags;
+ // XXX the VFS working directory is global state, this is unsafe.
+ Cmd.VFS->setCurrentWorkingDirectory(Cmd.Directory);
----------------
I assume that's a note to you that you want to change this?
================
Comment at: clangd/IncludeScanner.cpp:159
+ // When reusing flags, we add an explicit `-x cpp` to override the extension.
+ // TODO: copying CompilerInvocation would avoid this, and is more robust.
+ if (std::find(Cmd.CommandLine.begin(), Cmd.CommandLine.end(), "-x") ==
----------------
Wondering why we can't use one of the tooling abstractions here...
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41911/new/
https://reviews.llvm.org/D41911
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
