[PATCH] D81069: [clangd] Don't build diagnostics when preparing AST for a normal action.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:369
+      Inputs.Index && !BuildDir.getError()) {
     auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
     auto Inserter = std::make_shared<IncludeInserter>(
----------------
while here, maybe introduce another span for include fixer initialization ?

as `getFormatStyle` can do IO


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:374
     if (Preamble) {
       for (const auto &Inc : Preamble->Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
----------------
umm, actually this is wrong in presence of stale preambles ... we should perform patching before initializing include fixer and make use of patched includes rather than stale ones.

no action needing, just taking a note for myself.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:434
   Clang->getASTContext().setTraversalScope(ParsedDecls);
-  {
+  if (CTFinder.hasValue()) {
     // Run the AST-dependent part of the clang-tidy checks.
----------------
maybe
```
if (ProduceDiagnostics) {
 assert(CTFinder.hasValue());
...
}
```

to keep it similar to other usages.


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:113
     /// Indicates whether we reused the prebuilt AST.
+    /// FIXME: always false, diagnostics are always built from fresh ASTs.
     bool ReuseAST = false;
----------------
why don't we just drop this ? I believe this patch is doing 2 things:
- Don't build diagnostics for read operations
- Don't reuse cached asts for diagnostics

and I think getting rid of `ReuseAST` bit would belong to second one. (unless we decide to emit status while building asts for read operations)


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:470
+TEST(ParsedASTTest, NoDiagnostics) {
+  static bool TidyCheckIsError;
+  // A simple clang-tidy check, which we can verify:
----------------
nit: initialize to false


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:540
+  // Build one file in advance.
+  S.update(Foo, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
----------------
WantDiags::Auto should be enough here, as we are blocking afterwards. I've heard someone is tryin to get rid of those :P


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:546
+  // Build two more files.
+  S.update(Bar, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
+  S.update(Baz, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
----------------
again WantDiags::Auto should do for both


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:796
       [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   S.runWithAST("touchAST", FooCpp, [](Expected<InputsAndAST> IA) {
     // Make sure the AST was actually built.
----------------
is it worth asserting here (or in a new test) that AST.diags() is empty, whereas it is non-empty below ? (ofc, after introducing some diags to the code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81069