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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to