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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:167
+                                            std::move(VFS));
+  CI->getFrontendOpts().DisableFree = false;
+  return CI;
----------------
Why `DisableFree`?


================
Comment at: clangd/ClangdUnit.cpp:251
+        CompilerInstance::createDiagnostics(new DiagnosticOptions,
+                                            &CommandLineDiagsConsumer, false);
+    CI = createCompilerInvocation(ArgStrs, CommandLineDiagsEngine, VFS);
----------------
What's the purpose of this local scope here?
Wouldn't &CommandLineDiagsConsumer point to garbage after the end of this local scope?


================
Comment at: clangd/ClangdUnit.cpp:262
+      ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
+  if (!Preamble || !Preamble->Preamble.CanReuse(*CI, ContentsBuffer.get(),
+                                                Bounds, VFS.get())) {
----------------
We can not compute `Bounds`, `if (Preamble)`.


================
Comment at: clangd/ClangdUnit.cpp:268
+        CompilerInstance::createDiagnostics(
+            &CI->getDiagnosticOpts(), &PreambleDiagnosticsConsumer, false);
+    ClangdUnitPreambleCallbacks SerializedDeclsCollector;
----------------
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?


================
Comment at: clangd/ClangdUnit.cpp:404
+    CI = createCompilerInvocation(ArgStrs, CommandLineDiagsEngine, VFS);
+  }
+  assert(CI && "Couldn't create CompilerInvocation");
----------------
what's the purpose of this local scope?


================
Comment at: clangd/ClangdUnit.cpp:691
+
+void ClangdUnit::ParsedAST::ensurePreambleDeclsDeserialized() {
+  if (PendingTopLevelDecls.empty())
----------------
Why do we need to ensure that Decls are deserialized?


================
Comment at: clangd/ClangdUnit.h:77
+  /// Stores and provides access to parsed AST.
+  class ParsedAST {
+  public:
----------------
Why is this a separate class and not just part of `ClangdUnit`?


https://reviews.llvm.org/D35406