[PATCH] D94424: [clangd] Make AST-based signals available to runWithPreamble.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:858
+            std::string RelatedNS = OS.str();
+            if (!RelatedNS.empty()) {
+              Signals.RelatedNamespaces[RelatedNS + "::"] += 1;
----------------
nit: I think you can just use OS.str() here and below instead of RelatedNS to avoid extra string copy.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:716
+  };
+  } // namespace bar
+  )cpp";
----------------
bar or tar?


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:720
+  #include "foo.h"
+  namespace ns1 {
+  namespace ns2 {
----------------
Can you add anonymous namespaces as well?


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:733
+  S.update(Foo, getInputs(Foo, Contents), WantDiagnostics::Yes);
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
----------------
s/for/while?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94424/new/

https://reviews.llvm.org/D94424