[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.

[Manuel Klimek via Phabricator via cfe-commits]

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