[PATCH] D50695: [clangd] Always use the latest preamble

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, we were previously getting inconsistent ASTs (i.e. using different preambles), depending on whether they were built in `TUScheduler::update` or in `TUScheduler::runWithAST` handlers.
Just a few NITs.



================
Comment at: clangd/TUScheduler.cpp:373
       std::lock_guard<std::mutex> Lock(Mutex);
-      if (NewPreamble)
-        LastBuiltPreamble = NewPreamble;
+      // Always use the NewPreamble.
+      LastBuiltPreamble = NewPreamble;
----------------
Maybe remove the comment? It does not seem to add any additional information on top of the code.


================
Comment at: unittests/clangd/XRefsTests.cpp:1058
+  // stale one.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
----------------
Maybe use `runFindDefinitions` instead of the two `Server.findDefinitions()` + `Server.blockUntilIdleForTest()` calls?


================
Comment at: unittests/clangd/XRefsTests.cpp:1061
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc,
----------------
NIT: just use `cantFail(std::move(Loc))` to get the underlying vector and remove `ASSERT_TRUE`.
This gives equivalent results (crashes tests with `assert` failure internally in LLVM) with a simpler syntax.


================
Comment at: unittests/clangd/XRefsTests.cpp:1073
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
----------------
Same here: maybe use `runFindDefinitions`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695