kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/ASTSignals.cpp:28
+ // Source files: Use #import if the ObjC language flag is enabled
+ if (isHeaderFile(AST.tuPath(), AST.getLangOpts())) {
+ if (AST.getLangOpts().ObjC) {
----------------
can we rewrite this as:
```
bool preferImports(const ParsedAST &AST) {
// If we don't have any objc-ness, always prefer #include
if (!AST.getLangOpts().ObjC)
return false;
// If this is not a header file and has objc set as language, prefer #import
if (!isHeaderFile(...))
return true;
// Headers lack proper compile flags most of the time, so we might treat a
header as objc accidentally.
// Perform some extra checks to make sure this works.
// Any file with a #import, should keep #import-ing.
for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
if (Inc.Directive == tok::pp_import)
return true;
}
// Any file declaring an objc struct should also #import.
// No need to look over the references, as the file doesn't have any
#imports, it must be declaring interesting Objc-like decls.
for(Decl *D: AST.getLocalTopLevelDecls()) {
if (isa<...InterestingDeclTypes...>(D)
return true;
});
return false;
}
```
It's fine to run findExplicitReferences for an extra time here, as we do it
only when this is an ObjC header with no `#import`s
================
Comment at: clang-tools-extra/clangd/ASTSignals.h:33
llvm::StringMap<unsigned> RelatedNamespaces;
+ /// Whether include insertion should suggest imports or includes.
+ Symbol::IncludeDirective InsertionDirective =
----------------
can we rather say `Preferred PP-directive to use for inclusions by the file.`
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:569
}
+ Symbol::IncludeDirective Directive = Symbol::IncludeDirective::Include;
+ if (Signals)
----------------
we already have access to `Clang->getLangOpts()` here. we also have the
`FileName` available so we can check for header-ness as well. we also have
preamble includes to scan for #import directives (well, we might be building an
AST without a preamble sometimes but it's fine if IncludeFixer can't suggest
`#import`s in those cases).
so can we detect import-ness based on these here? we're only lacking
information about whether the file declares any objc symbols, e.g. we might
fail to suggest #import directives in a objc file without any #import
statements, but I'd say it's fine considering all the extra complexity needed
to propagate it from previous run. (if this turns out to be an important UX
issue we can consider plumbing this information directly, so please leave a
FIXME).
================
Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:91
+ HeaderTU.Code = R"cpp(
+ #include "TestTU.h"
+ )cpp";
----------------
can you put this test first with a comment like `objc lang option is not enough
for headers`?
================
Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:97
+ HeaderTU.Code = R"cpp(
+ #include "TestTU.h"
+
----------------
this only works because we actually implicitly `#import` contents of
`HeaderTU.HeaderCode` in TestTU.
can you instead clear up the `HeaderCode` and define the interface directly in
the header file?
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2727
- Results = completions("Fun^", {Sym}, {}, "Foo.m").Completions;
+ CodeCompleteOptions Opts;
+ Opts.ImportInsertions = true;
----------------
can you use this `Opts` (without Signals) in the previous test as well to make
sure we're `#include`ing the symbol in a non-import context even if the option
is set?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139458/new/
https://reviews.llvm.org/D139458
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
