[PATCH] D35406: [clangd] Replace ASTUnit with manual AST management.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:167
+                                            std::move(VFS));
+  CI->getFrontendOpts().DisableFree = false;
+  return CI;
----------------
krasimir wrote:
> Why `DisableFree`?
We rely on CompilerInstance to free the resources. It won't do that if `DisableFree` is set to true. Added a comment about that.


================
Comment at: clangd/ClangdUnit.cpp:251
+        CompilerInstance::createDiagnostics(new DiagnosticOptions,
+                                            &CommandLineDiagsConsumer, false);
+    CI = createCompilerInvocation(ArgStrs, CommandLineDiagsEngine, VFS);
----------------
krasimir wrote:
> What's the purpose of this local scope here?
> Wouldn't &CommandLineDiagsConsumer point to garbage after the end of this local scope?
This `DiagnosticsEngine` is not stored in `CompilerInvocation` created here, it is used for reporting errors during command-line parsing.
Since we don't show those errors to the user, we discard them.

Local scope is there to avoid referencing `CommandLineDiagsEngine` later (as it will discard diagnostics, and we actually want to have them).


================
Comment at: clangd/ClangdUnit.cpp:262
+      ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
+  if (!Preamble || !Preamble->Preamble.CanReuse(*CI, ContentsBuffer.get(),
+                                                Bounds, VFS.get())) {
----------------
krasimir wrote:
> We can not compute `Bounds`, `if (Preamble)`.
When `Preamble` is set, we need `Bounds` for `Preamble.CanReuse`.
When `Preamble` is not set, we need `Bounds` for `PrecompiledPreamble::Build`.

So we need them in both cases.


================
Comment at: clangd/ClangdUnit.cpp:268
+        CompilerInstance::createDiagnostics(
+            &CI->getDiagnosticOpts(), &PreambleDiagnosticsConsumer, false);
+    ClangdUnitPreambleCallbacks SerializedDeclsCollector;
----------------
krasimir wrote:
> Here `PreambleDiagsEngine` holds a pointer to the local variable `PreambleDiagnosticsConsumer`.
> Next, `BuiltPreamble` holds a reference to `PreambleDiagsEngine`.
> Next, `Preamble` is created by moving `BuiltPreamble`.
> Doesn't then `Preamble` hold a transitive reference to junk after this local scope?
I don't think `BuiltPreamble` holds a reference to `PreambleDiagsEngine`.  Here is a list of fields of `PrecompiledPreamble` class:


```
  //...
  TempPCHFile PCHFile;
  llvm::StringMap<PreambleFileHash> FilesInPreamble;
  std::vector<char> PreambleBytes;
  bool PreambleEndsAtStartOfLine;
```



================
Comment at: clangd/ClangdUnit.cpp:404
+    CI = createCompilerInvocation(ArgStrs, CommandLineDiagsEngine, VFS);
+  }
+  assert(CI && "Couldn't create CompilerInvocation");
----------------
krasimir wrote:
> what's the purpose of this local scope?
The `DiagnosticsEngine` used here uses default `DiagnosticOptions`.
A one that's properly configured after reading command-line arguments is created below, inside a `prepareCompilerInstance` call.
To avoid using `DiagnosticsEngine` configured by default, it is put into a local scope.


================
Comment at: clangd/ClangdUnit.cpp:691
+
+void ClangdUnit::ParsedAST::ensurePreambleDeclsDeserialized() {
+  if (PendingTopLevelDecls.empty())
----------------
krasimir wrote:
> Why do we need to ensure that Decls are deserialized?
These Decls are used in `findDefinitions` (specifically, they are passed to `indexTopLevelDecls`). I've opted for mimicing the `ASTUnit` behaviour here.

It is possible that visiting only local decls (i.e. not from `Preamble`) will not break anything. But that requires thorough testing and digging into how PCHs works (one question is, if it is possible to have decls from the same file as part of the PCH). I actually want to investigate if we actually need all that deserializing logic here, but would prefer to do that with a separate change to keep semantic changes related to this commit as small as possible.

PS. Alternatively, we could just visit all decls in `ASTContext` whenever we need them for `findDefinitions`, but [[ https://reviews.llvm.org/diffusion/L/browse/cfe/trunk/include/clang/Frontend/ASTUnit.h;308597$149 | a comment in ASTUnit]] says it's slower.


================
Comment at: clangd/ClangdUnit.h:77
+  /// Stores and provides access to parsed AST.
+  class ParsedAST {
+  public:
----------------
krasimir wrote:
> Why is this a separate class and not just part of `ClangdUnit`?
For managing the lifetime of an AST. Specifically:
 - there is a destructor here that calls `EndSourceFile` to free AST-related resources,
 - all getters return references to objects that either have pointers to resources, managed by `ParsedAST` (`getASTContext`, `getPreprocessor`, etc) or semantically tied to this specific parsing result(`getDiagnostics`, `PendingTopLevelDecls`).

`ClangdUnit` is managing two things at the moment: a preamble and an AST. Since the lifetime of those two is different, it is arguably a good idea to separate them.


https://reviews.llvm.org/D35406