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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
