llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Daniil Dudkin (unterumarmung) <details> <summary>Changes</summary> --- Patch is 25.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/196766.diff 10 Files Affected: - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-1) - (modified) clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h (+44-1) - (modified) clang-tools-extra/include-cleaner/lib/Analysis.cpp (+146-5) - (added) clang-tools-extra/include-cleaner/test/fragments-block-comment.cpp (+6) - (added) clang-tools-extra/include-cleaner/test/fragments-empty-comment.cpp (+6) - (modified) clang-tools-extra/include-cleaner/test/fragments-multi.cpp (+3-1) - (modified) clang-tools-extra/include-cleaner/test/fragments-spelled-path.cpp (+3-1) - (modified) clang-tools-extra/include-cleaner/test/fragments.cpp (+11-1) - (modified) clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp (+38-5) - (modified) clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp (+192) ``````````diff diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 22f760f616473..c508cd1bce510 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -654,7 +654,9 @@ Improvements to clang-include-cleaner ------------------------------------- - Added :program:`clang-include-cleaner` support for treating matching direct - includes as fragments of the main file with ``--fragment-headers``. + includes as fragments of the main file with ``--fragment-headers`` and for + optionally annotating preserved includes with + ``--fragment-dependency-comment-format``. Improvements to clang-include-fixer ----------------------------------- diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h index 1d28d87c025ca..268936cebd0d5 100644 --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -16,11 +16,13 @@ #include "clang/Format/Format.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/Preprocessor.h" +#include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include <functional> +#include <optional> #include <string> #include <vector> @@ -31,7 +33,6 @@ class Decl; class FileEntry; class HeaderSearch; namespace tooling { -class Replacements; struct IncludeStyle; } // namespace tooling namespace include_cleaner { @@ -119,12 +120,54 @@ AnalysisResults analyze(llvm::ArrayRef<Decl *> ASTRoots, const Preprocessor &PP, const AnalysisOptions &Options = {}); +enum class FragmentDependencyCommentStatus { + CanInsert, + AlreadyPresent, + ConflictingComment, +}; + +/// Planned comment state for an include kept alive by fragments. +struct FragmentDependencyComment { + const Include *Preserved = nullptr; + llvm::SmallVector<const Include *> Fragments; + std::string Text; + FragmentDependencyCommentStatus Status = + FragmentDependencyCommentStatus::CanInsert; + std::optional<tooling::Replacement> Replacement; +}; + +/// Replacements computed from include-cleaner findings. +struct IncludeFixes { + tooling::Replacements Replacements; + std::vector<FragmentDependencyComment> FragmentComments; +}; + +/// Options for turning analysis results into source edits. +struct FixIncludesOptions { + /// Raw trailing comment text without the leading //. + /// + /// When it contains `{0}`, that placeholder is replaced with a comma- + /// separated list of direct fragment include spellings that keep the include + /// alive. + llvm::StringRef FragmentDependencyCommentFormat; +}; + +/// Computes replacements to apply include-cleaner findings to the main file. +IncludeFixes computeIncludeFixes(const AnalysisResults &Results, + llvm::StringRef FileName, llvm::StringRef Code, + const format::FormatStyle &IncludeStyle, + const FixIncludesOptions &Options = {}); + /// Removes unused includes and inserts missing ones in the main file. /// Returns the modified main-file code. /// The FormatStyle must be C++ or ObjC (to support include ordering). std::string fixIncludes(const AnalysisResults &Results, llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle &IncludeStyle); +std::string fixIncludes(const AnalysisResults &Results, + llvm::StringRef FileName, llvm::StringRef Code, + const format::FormatStyle &IncludeStyle, + const FixIncludesOptions &Options); /// Gets all the providers for a symbol by traversing each location. /// Returned headers are sorted by relevance, first element is the most diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index 321b67a5787d3..34608af9b1189 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -379,19 +379,160 @@ AnalysisResults analyze(llvm::ArrayRef<Decl *> ASTRoots, return Results; } -std::string fixIncludes(const AnalysisResults &Results, - llvm::StringRef FileName, llvm::StringRef Code, - const format::FormatStyle &Style) { +namespace { + +// The fix planner only receives FileName and Code, so comment edits map +// Include::Line back to offsets in that text. +class LineIndex { +public: + explicit LineIndex(llvm::StringRef Code) : Code(Code) { + Starts.push_back(0); + for (unsigned I = 0; I < Code.size(); ++I) { + if (Code[I] == '\n') + Starts.push_back(I + 1); + } + Starts.push_back(Code.size()); + } + + llvm::StringRef line(unsigned OneBasedLine) const { + auto [Start, End] = bounds(OneBasedLine); + return Code.slice(Start, End); + } + + unsigned trimmedLineEnd(unsigned OneBasedLine) const { + auto [Start, End] = bounds(OneBasedLine); + while (End > Start && (Code[End - 1] == ' ' || Code[End - 1] == '\t')) + --End; + return End; + } + +private: + std::pair<unsigned, unsigned> bounds(unsigned OneBasedLine) const { + if (OneBasedLine == 0 || OneBasedLine >= Starts.size()) + return {Code.size(), Code.size()}; + unsigned Start = Starts[OneBasedLine - 1]; + unsigned End = + Starts[OneBasedLine] == 0 ? Code.size() : Starts[OneBasedLine] - 1; + if (End > Start && Code[End - 1] == '\r') + --End; + return {Start, End}; + } + + llvm::StringRef Code; + llvm::SmallVector<unsigned> Starts; +}; + +std::string formatFragmentDependencyComment( + llvm::StringRef Format, llvm::ArrayRef<const Include *> FragmentIncludes) { + if (Format.empty()) + return {}; + + std::string FragmentList; + for (const Include *Fragment : FragmentIncludes) { + if (!FragmentList.empty()) + FragmentList += ", "; + FragmentList += Fragment->quote(); + } + + std::string Result; + llvm::StringRef Remaining = Format; + while (true) { + auto Pos = Remaining.find("{0}"); + if (Pos == llvm::StringRef::npos) { + Result += Remaining.str(); + return Result; + } + Result += Remaining.take_front(Pos).str(); + Result += FragmentList; + Remaining = Remaining.drop_front(Pos + 3); + } +} + +FragmentDependencyComment inspectFragmentDependencyComment( + const FragmentDependency &Dependency, llvm::StringRef FileName, + const LineIndex &Lines, llvm::StringRef CommentFormat) { + FragmentDependencyComment Comment{ + Dependency.Preserved, Dependency.Fragments, + formatFragmentDependencyComment(CommentFormat, Dependency.Fragments), + FragmentDependencyCommentStatus::CanInsert, std::nullopt}; + if (Comment.Text.empty()) + return Comment; + + llvm::StringRef Line = Lines.line(Dependency.Preserved->Line); + size_t IncludePos = Line.find(Dependency.Preserved->quote()); + if (IncludePos == llvm::StringRef::npos) { + Comment.Status = FragmentDependencyCommentStatus::ConflictingComment; + return Comment; + } + + llvm::StringRef Tail = + Line.drop_front(IncludePos + Dependency.Preserved->quote().size()); + llvm::StringRef TrimmedTail = Tail.ltrim(" \t"); + if (TrimmedTail.empty()) { + Comment.Status = FragmentDependencyCommentStatus::CanInsert; + Comment.Replacement = tooling::Replacement( + FileName, Lines.trimmedLineEnd(Dependency.Preserved->Line), 0, + " // " + Comment.Text); + return Comment; + } + + if (TrimmedTail.starts_with("//")) { + llvm::StringRef Existing = TrimmedTail.drop_front(2).trim(); + Comment.Status = Existing == Comment.Text + ? FragmentDependencyCommentStatus::AlreadyPresent + : FragmentDependencyCommentStatus::ConflictingComment; + return Comment; + } + + Comment.Status = FragmentDependencyCommentStatus::ConflictingComment; + return Comment; +} + +} // namespace + +IncludeFixes computeIncludeFixes(const AnalysisResults &Results, + llvm::StringRef FileName, llvm::StringRef Code, + const format::FormatStyle &Style, + const FixIncludesOptions &Options) { assert(Style.isCpp() && "Only C++ style supports include insertions!"); - tooling::Replacements R; + IncludeFixes Fixes; + tooling::Replacements &R = Fixes.Replacements; // Encode insertions/deletions in the magic way clang-format understands. for (const Include *I : Results.Unused) cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote()))); for (const MissingInclude &Missing : Results.MissingIncludes) cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0, "#include " + Missing.Spelling))); + + if (Options.FragmentDependencyCommentFormat.empty()) + return Fixes; + + LineIndex Lines(Code); + for (const FragmentDependency &Dependency : Results.FragmentDependencies) { + FragmentDependencyComment Comment = inspectFragmentDependencyComment( + Dependency, FileName, Lines, Options.FragmentDependencyCommentFormat); + if (Comment.Replacement) + cantFail(R.add(*Comment.Replacement)); + Fixes.FragmentComments.push_back(std::move(Comment)); + } + return Fixes; +} + +std::string fixIncludes(const AnalysisResults &Results, + llvm::StringRef FileName, llvm::StringRef Code, + const format::FormatStyle &Style) { + return fixIncludes(Results, FileName, Code, Style, {}); +} + +std::string fixIncludes(const AnalysisResults &Results, + llvm::StringRef FileName, llvm::StringRef Code, + const format::FormatStyle &Style, + const FixIncludesOptions &Options) { + IncludeFixes Fixes = + computeIncludeFixes(Results, FileName, Code, Style, Options); // "cleanup" actually turns the UINT_MAX replacements into concrete edits. - auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style)); + auto Positioned = cantFail( + format::cleanupAroundReplacements(Code, Fixes.Replacements, Style)); return cantFail(tooling::applyAllReplacements(Code, Positioned)); } diff --git a/clang-tools-extra/include-cleaner/test/fragments-block-comment.cpp b/clang-tools-extra/include-cleaner/test/fragments-block-comment.cpp new file mode 100644 index 0000000000000..fc2a004fbe324 --- /dev/null +++ b/clang-tools-extra/include-cleaner/test/fragments-block-comment.cpp @@ -0,0 +1,6 @@ +#include <vector> /* keep me */ +#include "gen.inc" + +Gen G; + +// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' --fragment-dependency-comment-format='needed by {0}' -- -I%S/Inputs/ | count 0 diff --git a/clang-tools-extra/include-cleaner/test/fragments-empty-comment.cpp b/clang-tools-extra/include-cleaner/test/fragments-empty-comment.cpp new file mode 100644 index 0000000000000..0e7ee5615e7d3 --- /dev/null +++ b/clang-tools-extra/include-cleaner/test/fragments-empty-comment.cpp @@ -0,0 +1,6 @@ +#include <vector> +#include "gen.inc" + +Gen G; + +// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' -- -I%S/Inputs/ | count 0 diff --git a/clang-tools-extra/include-cleaner/test/fragments-multi.cpp b/clang-tools-extra/include-cleaner/test/fragments-multi.cpp index 5aa1814df7326..89cfd96c4350f 100644 --- a/clang-tools-extra/include-cleaner/test/fragments-multi.cpp +++ b/clang-tools-extra/include-cleaner/test/fragments-multi.cpp @@ -5,4 +5,6 @@ A AValue; B BValue; -// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' -- -I%S/Inputs/ | count 0 +// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' --fragment-dependency-comment-format='needed by {0}' -- -I%S/Inputs/ | FileCheck --check-prefix=CHANGES %s +// CHANGES: ~ <vector> @Line:1 // needed by "a.inc", "b.inc" +// CHANGES-NOT: - <vector> diff --git a/clang-tools-extra/include-cleaner/test/fragments-spelled-path.cpp b/clang-tools-extra/include-cleaner/test/fragments-spelled-path.cpp index 82d0d27c34df9..45174240ab498 100644 --- a/clang-tools-extra/include-cleaner/test/fragments-spelled-path.cpp +++ b/clang-tools-extra/include-cleaner/test/fragments-spelled-path.cpp @@ -3,4 +3,6 @@ SpelledGen G; -// RUN: clang-include-cleaner -print=changes %s --fragment-headers='generated/gen\.inc' -- -I%S/Inputs/ | count 0 +// RUN: clang-include-cleaner -print=changes %s --fragment-headers='generated/gen\.inc' --fragment-dependency-comment-format='needed by {0}' -- -I%S/Inputs/ | FileCheck --check-prefix=CHANGES %s +// CHANGES: ~ <vector> @Line:1 // needed by "generated/gen.inc" +// CHANGES-NOT: - <vector> diff --git a/clang-tools-extra/include-cleaner/test/fragments.cpp b/clang-tools-extra/include-cleaner/test/fragments.cpp index 0e7ee5615e7d3..159fc5736984c 100644 --- a/clang-tools-extra/include-cleaner/test/fragments.cpp +++ b/clang-tools-extra/include-cleaner/test/fragments.cpp @@ -3,4 +3,14 @@ Gen G; -// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' -- -I%S/Inputs/ | count 0 +// RUN: clang-include-cleaner -print=changes %s --fragment-headers='.*\.inc$' --fragment-dependency-comment-format='needed by {0}' -- -I%S/Inputs/ | FileCheck --check-prefix=CHANGES %s +// CHANGES: ~ <vector> @Line:1 // needed by "gen.inc" +// CHANGES-NOT: - <vector> + +// RUN: clang-include-cleaner -print %s --fragment-headers='.*\.inc$' --fragment-dependency-comment-format='needed by {0}' -- -I%S/Inputs/ | FileCheck --check-prefix=PRINT %s +// PRINT: #include <vector> // needed by "gen.inc" + +// RUN: cp %s %t.cpp +// RUN: clang-include-cleaner -edit %t.cpp --fragment-headers='.*\.inc$' --fragment-dependency-comment-format='IWYU pragma: keep' -- -I%S/Inputs/ +// RUN: FileCheck --check-prefix=EDIT %s < %t.cpp +// EDIT: #include <vector> // IWYU pragma: keep diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp index fbc14be848b5f..675e59a37c601 100644 --- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -88,6 +88,15 @@ cl::opt<std::string> FragmentHeaders{ cl::cat(IncludeCleaner), }; +cl::opt<std::string> FragmentDependencyCommentFormat{ + "fragment-dependency-comment-format", + cl::desc("Append this trailing comment to includes that are used only by " + "fragment headers. Use {0} for the matched fragment include " + "spellings."), + cl::init(""), + cl::cat(IncludeCleaner), +}; + enum class PrintStyle { Changes, Final }; cl::opt<PrintStyle> Print{ "print", @@ -147,9 +156,12 @@ class Action : public clang::ASTFrontendAction { public: Action(std::function<bool(const Header &)> HeaderFilter, std::function<bool(llvm::StringRef)> FragmentHeaderFilter, + std::string FragmentDependencyCommentFormat, llvm::StringMap<std::string> &EditedFiles) : HeaderFilter(std::move(HeaderFilter)), FragmentHeaderFilter(std::move(FragmentHeaderFilter)), + FragmentDependencyCommentFormat( + std::move(FragmentDependencyCommentFormat)), EditedFiles(EditedFiles) {} private: @@ -158,6 +170,7 @@ class Action : public clang::ASTFrontendAction { PragmaIncludes PI; std::function<bool(const Header &)> HeaderFilter; std::function<bool(llvm::StringRef)> FragmentHeaderFilter; + std::string FragmentDependencyCommentFormat; llvm::StringMap<std::string> &EditedFiles; bool BeginInvocation(CompilerInstance &CI) override { @@ -237,7 +250,14 @@ class Action : public clang::ASTFrontendAction { Results.MissingIncludes.clear(); if (!Remove || DisableRemove) Results.Unused.clear(); - std::string Final = fixIncludes(Results, AbsPath, Code, getStyle(AbsPath)); + format::FormatStyle Style = getStyle(AbsPath); + FixIncludesOptions FixOptions{FragmentDependencyCommentFormat}; + IncludeFixes Fixes = + computeIncludeFixes(Results, AbsPath, Code, Style, FixOptions); + auto Positioned = cantFail( + format::cleanupAroundReplacements(Code, Fixes.Replacements, Style)); + std::string Final = + cantFail(tooling::applyAllReplacements(Code, Positioned)); if (Print.getNumOccurrences()) { switch (Print) { @@ -246,6 +266,13 @@ class Action : public clang::ASTFrontendAction { llvm::outs() << "- " << I->quote() << " @Line:" << I->Line << "\n"; for (const MissingInclude &I : Results.MissingIncludes) llvm::outs() << "+ " << I.Spelling << "\n"; + for (const auto &Comment : Fixes.FragmentComments) { + if (!Comment.Replacement) + continue; + llvm::outs() << "~ " << Comment.Preserved->quote() + << " @Line:" << Comment.Preserved->Line << " // " + << Comment.Text << "\n"; + } break; case PrintStyle::Final: llvm::outs() << Final; @@ -253,7 +280,7 @@ class Action : public clang::ASTFrontendAction { } } - if (!Results.MissingIncludes.empty() || !Results.Unused.empty()) + if (!Fixes.Replacements.empty()) EditedFiles.try_emplace(AbsPath, Final); } @@ -274,12 +301,16 @@ class Action : public clang::ASTFrontendAction { class ActionFactory : public tooling::FrontendActionFactory { public: ActionFactory(std::function<bool(const Header &)> HeaderFilter, - std::function<bool(llvm::StringRef)> FragmentHeaderFilter) + std::function<bool(llvm::StringRef)> FragmentHeaderFilter, + std::string FragmentDependencyCommentFormat) : HeaderFilter(std::move(HeaderFilter)), - FragmentHeaderFilter(std::move(FragmentHeaderFilter)) {} + FragmentHeaderFilter(std::move(FragmentHeaderFilter)), + FragmentDependencyCommentFormat( + std::move(FragmentDependencyCommentFormat)) {} std::unique_ptr<clang::FrontendAction> create() override { return std::make_unique<Action>(HeaderFilter, FragmentHeaderFilter, + FragmentDependencyCommentFormat, EditedFiles); } @@ -290,6 +321,7 @@ class ActionFactory : public tooling::FrontendActionFactory { private: std::function<bool(const Header &)> HeaderFilter; std::function<bool(llvm::StringRef)> FragmentHeaderFilter; + std::string FragmentDependencyCommentFormat; // Map from file name to final code with the include edits applied. llvm::StringMap<std::string> EditedFiles; }; @@ -431,7 +463,8 @@ int main(int argc, const char **argv) { auto FragmentHeaderFilter = fragmentHeaderFilter(); if (!HeaderFilter || !FragmentHeaderFilter) return 1; // error already reported. - ActionFactory Factory(HeaderFilter, FragmentHeaderFilter); + ActionFactory Factory(HeaderFilter, FragmentHeaderFilter, + FragmentDependencyCommentFormat); auto ErrorCode = Tool.run(&Factory); if (Edit) { for (const auto &NameAndContent : Factory.editedFiles()) { diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp index 0c68fb58032fb..64a4fb133bf22 100644 --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -812,6 +812,198 @@ TEST(FixIncludes, Basic) { #include "a.h")cpp"); } +TEST(FixIncludes, FragmentDependencyComments) { + llvm::StringRef Code = R"cpp(#include "a.h" +#include "gen.inc" +#include "existing.h" // needed by "gen.inc" +#include "conflict.h" // keep me +)cpp"; + + Includes Inc; + Include A; + A.Spelled = "a.h"; + A.Line = 1; + Inc.add(A); + Include Gen; + Gen.Spelled = "gen.inc"; + Gen.Line = 2; + Inc.add(Gen); + Include Existing; + Existing.Spelled = "existing.h"; + Existing.Line = 3; + Inc.add(Existing); + Include Conflict; + Conflict.Spelled = "conflict.h"; + Conflict.Line = 4; + Inc.add(Conflict); ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/196766 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
