[PATCH] D73916: [clang] Add `forceReload` clangd extension to 'textDocument/didChange'

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Looks pretty good now, thanks! Test needs to be more precise (doesn't actually test the behavior at present, I think).



================
Comment at: clang-tools-extra/clangd/Compiler.h:41
   bool SuggestMissingIncludes = false;
+  bool ForceRebuild = false;
 };
----------------
Hmm, on thinking further I do think this belongs in ParseInputs next to Contents or CompileCommands, as these things are things that affect the parse but *don't* invalidate caches.


================
Comment at: clang-tools-extra/clangd/Compiler.h:41
   bool SuggestMissingIncludes = false;
+  bool ForceRebuild = false;
 };
----------------
sammccall wrote:
> Hmm, on thinking further I do think this belongs in ParseInputs next to Contents or CompileCommands, as these things are things that affect the parse but *don't* invalidate caches.
this is a good place for a comment explaining what this does specifically (prevents reuse of cached preamble/ast)


================
Comment at: clang-tools-extra/clangd/Protocol.h:660
+  /// This is a clangd extension.
+  bool forceRebuild;
 };
----------------
`= false`, and remove "disabled by default" from comment


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:619
 
+TEST_F(TUSchedulerTests, ForceRebuild) {
+  TUScheduler S(CDB, optsForTest(), captureDiags());
----------------
This test doesn't actually verify that the preamble was rebuilt, just that the AST was.
Checking the actual diagnostics would do that.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:631
+  // Return value indicates if the updated callback was received.
+  auto DoUpdate = [&](std::string Contents, bool ForceRebuild) -> bool {
+    std::atomic<bool> Updated(false);
----------------
I don't think this abstraction is useful since you just have 3 calls and two of them need the actual diagnostics.

You could have diag callbacks inline easily enough:

```
vector<vector<Diag>> diags;
update(..., [] { diags.push_back(...) })
update(..., [] { ADD_FAILURE(...) })
update(..., [] { diags.push_back(...) })
blockUntilIdle();
EXPECT_THAT(diags, ElementsAre(
  /* First */ElementsAre(...missing header...),
  /* Last */IsEmpty());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73916