kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/Config.h:91 + enum class MissingIncludesPolicy { + /// Diagnose missing includes. ---------------- rather than duplicating, what about renaming `UnusedIncludesPolicy` to `IncludesPolicy` and use it for both `UnusedIncludes` and `MissingIncludes` options below? ================ Comment at: clang-tools-extra/clangd/ConfigFragment.h:224 - /// Controls how clangd will correct "unnecessary #include directives. + /// Controls how clangd will correct "unnecessary" #include directives. /// clangd can warn if a header is `#include`d but not used, and suggest ---------------- nit: i'd rather drop the quotes completely to match your description of MissingIncludes below. ================ Comment at: clang-tools-extra/clangd/ConfigFragment.h:238 + /// Controls how clangd handles missing #include directives. + /// clangd can warn if a header for a symbol is not `#include`d (missing), ---------------- ================ Comment at: clang-tools-extra/clangd/ConfigFragment.h:239-240 + /// Controls how clangd handles missing #include directives. + /// clangd can warn if a header for a symbol is not `#include`d (missing), + /// and suggest adding it. + /// ---------------- ================ Comment at: clang-tools-extra/clangd/ConfigFragment.h:242 + /// + /// Strict means a header is missing if it is not *directly #include'd. + /// The file might still compile if the header is included transitively. ---------------- ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:516 + + std::vector<include_cleaner::SymbolReference> Macros = + collectMacroReferences(AST); ---------------- ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:547 +convertIncludes(const SourceManager &SM, + std::vector<Inclusion> MainFileIncludes) { + include_cleaner::Includes Includes; ---------------- you can just pass an `llvm::ArrayRef<Inclusion>` to prevent a copy ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:550 + for (const Inclusion &Inc : MainFileIncludes) { + llvm::ErrorOr<const FileEntry *> ResolvedOrError = + SM.getFileManager().getFile(Inc.Resolved); ---------------- you can re-write this as: ``` include_cleaner::Include TransformedInc; TransformedInc.Spelled = Inc.Written.trim("\"<>"); TransformedInc.HashLocation = SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); // we should actually convert this from a SourceLocation to offset in include_cleaner::Include as well TransformedInc.Line = Inc.HashLine; TransformedInc.Angled = WrittenRef.starts_with("<"); if(auto FE = SM.getFileManager().getFile(Inc.Resolved)) TransformedInc.Resolved = *FE; Includes.add(std::move(TransformedInc)); ``` ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:570 +computeMissingIncludes(ParsedAST &AST) { + std::vector<include_cleaner::SymbolReference> Macros = + collectMacroReferences(AST); ---------------- nit: `auto Macros = ..` ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:572 + collectMacroReferences(AST); + std::vector<Inclusion> MainFileIncludes = + AST.getIncludeStructure().MainFileIncludes; ---------------- no need for copying to vector here, `const auto& MainFileIncludes = ...` ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:577 + convertIncludes(AST.getSourceManager(), MainFileIncludes); + std::string FileName = + AST.getSourceManager() ---------------- you can use `AST.tuPath()` ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:582 + .str(); + if (FileName.find("foo") != std::string::npos) { + vlog("Include cleaner includes: {0}", IncludeCleanerIncludes.all().size()); ---------------- looks like debugging artifact? ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:593 + llvm::ArrayRef<include_cleaner::Header> Providers) { + bool Satisfied = false; + for (const include_cleaner::Header &H : Providers) { ---------------- nit: you can check whether `Ref.RT` is `Explicit` at the top, and bail out early. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:597 + H.physical() == MainFile) { + Satisfied = true; + } ---------------- nit: you can just `break` after satisfying the include (same below) ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:605 + Ref.RT == include_cleaner::RefType::Explicit) { + std::string SpelledHeader = include_cleaner::spellHeader( + Providers.front(), AST.getPreprocessor().getHeaderSearchInfo(), ---------------- clangd has some header spelling customizations. so we should actually be doing this through `URI::includeSpelling(URI::create(getCanonicalPath(Providers.front().physical(), SM)))` first, and fall back to `spellHeader` if it fails for physical header providers to make sure we're consistent. this is used by $EMPLOYER$'s integration to always spell includes relative to depot root, rather than certain include search paths. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:609 + std::pair<FileID, unsigned int> FileIDAndOffset = + AST.getSourceManager().getDecomposedLoc(Ref.RefLocation); + Missing.push_back({SpelledHeader, FileIDAndOffset.second}); ---------------- `RefLocation` is not necessarily spelled token location, e.g. it might be pointing at a macro expansion. you can make use of `TokenBuffer` inside `ParsedAST` to go from this expanded location to a token spelled inside the main file, e.g. `Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation));` and use the full spelled token range afterwards for diagnostic location. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:661 + D.Fixes.back().Message = "#include " + Missing.first; + D.Fixes.back().Edits.emplace_back(); + D.Fixes.back().Edits.back().newText = "#include " + Missing.first + "\n"; ---------------- we've got some tooling library to generate these edits, see https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Tooling/Inclusions/HeaderIncludes.h#L76. that way they'll be placed in "correct" position among the existing includes. you can then use [replacementToEdit](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SourceCode.h#L148) to convert into a clangd edit. ================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:695 + auto MissingHeadersDiags = + issueMissingIncludesDiagnostics(Result, Inputs.Contents); Result.Diags->insert(Result.Diags->end(), ---------------- instead of introducing a new function here and duplicating logic what about something like: ``` auto IncludeDiags = issueIncludeDiagnostics(...); ``` in IncludeCleaner.cpp: ``` struct IncludeCleanerFindings { std::vector<Inclusion*> Unused; llvm::StringMap<std::vector<Range>> MissingIncludes; // Map from missing header spelling to ranges of references. }; IncludeCleanerFindings computeIncludeCleanerFindings(AST) { auto Incs = convertIncludes(); walkUsed(...., { // Update Missing includes, mark relevant main file includes as used. }); // Create the unused includes set return Findings; } std::vector<Diag> issueIncludeDiagnostics(...) { std::vector<Inclusion*> UnusedIncludes; llvm::StringMap<std::vector</*Location of reference*/clangd::Range>> MissingIncludes; if (UnusedIncludePolicy is Strict) { UnusedIncludes = computeUnusedIncludes(...); } else if(UnusedIncludesPolicy is Experiment || MissingIncludesPolicy is Strict) { auto Findings = computeIncludeCleanerFindings(AST); if(UnusedIncludesPolicy is Experiment) { UnusedIncludes = std::move(Findings.Unused); } if(MissingIncludesPolicy is Strict) { MissingIncludes = std::move(Findings.Missing); } } for(auto &Inc: UnusedIncludes) { ... // create unused include diag. } for(auto &Missing: MissingIncludes) { // create missing include diag. } return Diags; } ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143496/new/ https://reviews.llvm.org/D143496 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits