[PATCH] D139458: [clangd] Full support for #import insertions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/AST.h:122
 
+/// Returns the Include Directive which should be used for include insertions
+/// for the given main file.
----------------
Maybe something more concise like:
```
Infer the include directive to use for the given \p FileName. It aims for #import for ObjC files and #include for the rest.

For source files we use LangOpts directly to infer ObjC-ness.
For header files we also check for symbols declared by the file and existing include directives, as the language can be set to objc as a fallback in the absence of compile flags.
```

To make sure we're first talking about "what" it's trying to achieve and then talk about "how". That way future travelers can verify their changes to heuristics to see if they're breaking the semantics.


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:88
   /// (These should be const, but RecursiveASTVisitor requires Decl*).
-  ArrayRef<Decl *> getLocalTopLevelDecls();
+  ArrayRef<Decl *> getLocalTopLevelDecls() const;
 
----------------
dgoldman wrote:
> kadircet wrote:
> > can you rather introduce a new overload that returns `ArrayRef<const Decl*>`
> Did you mean an overload with `ArrayRef<const Decl *> getLocalTopLevelDecls() const;`? ASTSignals gets a const ParsedAST &, unless you want me to const cast instead?
yes I meant exactly that signature.


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:622
 
+Symbol::IncludeDirective preferredIncludeDirective(TestTU &TU) {
+  auto AST = TU.build();
----------------
nit: I'd make this a lambda in the test below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139458/new/

https://reviews.llvm.org/D139458