sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Just a bunch of nits, up to you to work out how/whether to address.
One substantive issue around classes defined outside headers - will chat
offline.
================
Comment at: clangd/ClangdServer.h:118
+
+ bool EnableIncludeFixer = false;
};
----------------
Can we call this option `SuggestMissingIncludes` or so? include-fixer is the
name of a separate tool.
================
Comment at: clangd/ClangdUnit.cpp:295
+ llvm::Optional<IncludeFixer> FixIncludes;
+ auto BuildDir = VFS->getCurrentWorkingDirectory();
----------------
sammccall wrote:
> Probably worth a two-line comment explaining at a high level what this is
> doing.
> Somewhat redundant with `IncludeFixer.h` but probably worth it anyway since
> this containing function is a complicated mess.
nit: can you hoist this above the line declaring FixIncludes?
That way (I think) it reads more naturally as "this is what the following block
of code does" in a big method, doesn't fold away with the if, etc
================
Comment at: clangd/ClangdUnit.cpp:305
+ }
+ FixIncludes.emplace(MainInput.getFile(), std::move(Inserter), *Index);
+ ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,
----------------
nit: I'm not sure I agree with the IncludeFixer class taking ownership of the
include inserter.
It reads it but never writes to it, so full ownership isn't *needed*. And if we
had other features that wanted to compute include insertions, we'd want them to
share an inserter.
================
Comment at: clangd/Compiler.h:49
+ // Used to recover from diagnostics (e.g. find missing includes for symbol).
+ llvm::Optional<const SymbolIndex *> Index;
+ ParseOptions Opts;
----------------
isn't an optional pointer just a pointer?
================
Comment at: clangd/IncludeFixer.cpp:39
+ const clang::Diagnostic &Info) const {
+ if (IndexRequestCount >= 5)
+ return {}; // Avoid querying index too many times in a single parse.
----------------
consider moving this 5 to a constructor param, so this file can be entirely
mechanism and the policy is spelled out at the construction site.
(No need to actually make it configurable, I think)
================
Comment at: clangd/IncludeFixer.cpp:41
+ return {}; // Avoid querying index too many times in a single parse.
+ if (isIncompleteTypeDiag(Info.getID())) {
+ // Incomplete type diagnostics should have a QualType argument for the
----------------
(You could also just write this as just a switch on Info.getID(), I'd find that
a little direct/easier to read, up to you)
================
Comment at: clangd/IncludeFixer.cpp:61
+ return {};
+ std::string IncompleteType = printQualifiedName(*TD);
+ if (IncompleteType.empty()) {
----------------
TypeName?
================
Comment at: clangd/IncludeFixer.cpp:63
+ if (IncompleteType.empty()) {
+ vlog("No incomplete type name is found in diagnostic. Ignore.");
+ return {};
----------------
this seems an odd thing to check and way to phrase - there *was* a type in the
message, but its name happens to be empty. Am I missing something? (I'd suggest
just removing this)
================
Comment at: clangd/IncludeFixer.cpp:80
+ Index.lookup(Req, [&](const Symbol &Sym) {
+ // FIXME: support multiple matched symbols.
+ if (Matched || (Sym.Scope + Sym.Name).str() != IncompleteType)
----------------
fixme is obsolete
================
Comment at: clangd/IncludeFixer.cpp:81
+ // FIXME: support multiple matched symbols.
+ if (Matched || (Sym.Scope + Sym.Name).str() != IncompleteType)
+ return;
----------------
no need to check name
================
Comment at: clangd/IncludeFixer.cpp:86
+
+ if (!Matched || Matched->IncludeHeaders.empty())
+ return {};
----------------
There's a case I'm worried about:
Foo.h declares `class X;`, Foo.cpp defines `class X{}`.
Now Bar.cpp tries to do `sizeof(X)`, so we get an incomplete type diagnostic.
- suggesting `#include Foo.cpp` is bad - can't include impl file
- suggesting `#include Foo.h` is also bad - doesn't help
- providing no suggestion is correct.
I'm not sure the index really has enough structured information to detect this
case, but we could check e.g. that definition header == canonical declaration
header? Or that basename(IncludeHeaders) == basename(definition header)?
If possible, can you add a test for this case?
================
Comment at: clangd/IncludeFixer.h:39
+ /// Attempts to recover diagnostic caused by an incomplete type \p T.
+ std::vector<Fix> fixInCompleteType(const Type &T) const;
+
----------------
sammccall wrote:
> nit: incomplete is one word
not done - method is still "fixInCompleteType"
================
Comment at: clangd/tool/ClangdMain.cpp:210
+static llvm::cl::opt<bool> EnableIncludeFixer(
+ "include-fixer",
----------------
similarly, I'd call this -suggest-missing-includes or just -suggest-includes
================
Comment at: clangd/tool/ClangdMain.cpp:214
+ "includes using index."),
+ llvm::cl::init(false), llvm::cl::Hidden);
+
----------------
why hidden?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56903/new/
https://reviews.llvm.org/D56903
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
