[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7f3d2cd7ec25: [include-cleaner] Fix the `fixIncludes` API not respect main-file header. (authored by hokein). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154950/new/ https://reviews.llvm.org/D154950 Files: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,11 +300,22 @@ Results.Unused.push_back(Inc.atLine(3)); Results.Unused.push_back(Inc.atLine(4)); - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" #include "a.h" #include "aa.h" #include "ab.h" #include +)cpp"); + + Results = {}; + Results.Missing.push_back("\"d.h\""); + Code = R"cpp(#include "a.h")cpp"; + // FIXME: this isn't correct, the main-file header d.h should be added before + // a.h. + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "a.h" +#include "d.h" )cpp"); } Index: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp === --- clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -164,7 +164,7 @@ Results.Missing.clear(); if (!Remove) Results.Unused.clear(); -std::string Final = fixIncludes(Results, Code, getStyle(Path)); +std::string Final = fixIncludes(Results, Path, Code, getStyle(Path)); if (Print.getNumOccurrences()) { switch (Print) { Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp === --- clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -112,15 +112,16 @@ return Results; } -std::string fixIncludes(const AnalysisResults , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ) { assert(Style.isCpp() && "Only C++ style supports include insertions!"); tooling::Replacements R; // Encode insertions/deletions in the magic way clang-format understands. for (const Include *I : Results.Unused) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 1, I->quote(; +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote(; for (llvm::StringRef Spelled : Results.Missing) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 0, +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0, ("#include " + Spelled).str(; // "cleanup" actually turns the UINT_MAX replacements into concrete edits. auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style)); Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h === --- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -79,7 +79,8 @@ /// 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 , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ); /// Gets all the providers for a symbol by traversing each location. Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,11 +300,22 @@
[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.
hokein added inline comments. Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:303 - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" kadircet wrote: > can you also have a test in which we insert "d.h" and it gets put into the > right place? added a test with a FIXME -- unfortunately, this case doesn't work yet (I believe this is a bug in the `tooling::HeaderIncludes`, I plan to fix it afterwards.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154950/new/ https://reviews.llvm.org/D154950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.
hokein updated this revision to Diff 539045. hokein added a comment. add a testcase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154950/new/ https://reviews.llvm.org/D154950 Files: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,11 +300,22 @@ Results.Unused.push_back(Inc.atLine(3)); Results.Unused.push_back(Inc.atLine(4)); - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" #include "a.h" #include "aa.h" #include "ab.h" #include +)cpp"); + + Results = {}; + Results.Missing.push_back("\"d.h\""); + Code = R"cpp(#include "a.h")cpp"; + // FIXME: this isn't correct, the main-file header d.h should be added before + // a.h. + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "a.h" +#include "d.h" )cpp"); } Index: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp === --- clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -164,7 +164,7 @@ Results.Missing.clear(); if (!Remove) Results.Unused.clear(); -std::string Final = fixIncludes(Results, Code, getStyle(Path)); +std::string Final = fixIncludes(Results, Path, Code, getStyle(Path)); if (Print.getNumOccurrences()) { switch (Print) { Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp === --- clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -112,15 +112,16 @@ return Results; } -std::string fixIncludes(const AnalysisResults , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ) { assert(Style.isCpp() && "Only C++ style supports include insertions!"); tooling::Replacements R; // Encode insertions/deletions in the magic way clang-format understands. for (const Include *I : Results.Unused) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 1, I->quote(; +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote(; for (llvm::StringRef Spelled : Results.Missing) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 0, +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0, ("#include " + Spelled).str(; // "cleanup" actually turns the UINT_MAX replacements into concrete edits. auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style)); Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h === --- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -79,7 +79,8 @@ /// 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 , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ); /// Gets all the providers for a symbol by traversing each location. Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,11 +300,22 @@ Results.Unused.push_back(Inc.atLine(3)); Results.Unused.push_back(Inc.atLine(4)); - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code,
[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.
kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:303 - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" can you also have a test in which we insert "d.h" and it gets put into the right place? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154950/new/ https://reviews.llvm.org/D154950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.
hokein created this revision. hokein added a reviewer: kadircet. Herald added a project: All. hokein requested review of this revision. Herald added a project: clang-tools-extra. The fixIncludes was using the `input` as the main file path, this will results in inserting header at wrong places. We need the main file path to so that we can get the real main-file header. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D154950 Files: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,7 +300,8 @@ Results.Unused.push_back(Inc.atLine(3)); Results.Unused.push_back(Inc.atLine(4)); - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" #include "a.h" #include "aa.h" #include "ab.h" Index: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp === --- clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -164,7 +164,7 @@ Results.Missing.clear(); if (!Remove) Results.Unused.clear(); -std::string Final = fixIncludes(Results, Code, getStyle(Path)); +std::string Final = fixIncludes(Results, Path, Code, getStyle(Path)); if (Print.getNumOccurrences()) { switch (Print) { Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp === --- clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -112,15 +112,16 @@ return Results; } -std::string fixIncludes(const AnalysisResults , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ) { assert(Style.isCpp() && "Only C++ style supports include insertions!"); tooling::Replacements R; // Encode insertions/deletions in the magic way clang-format understands. for (const Include *I : Results.Unused) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 1, I->quote(; +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote(; for (llvm::StringRef Spelled : Results.Missing) -cantFail(R.add(tooling::Replacement("input", UINT_MAX, 0, +cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0, ("#include " + Spelled).str(; // "cleanup" actually turns the UINT_MAX replacements into concrete edits. auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style)); Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h === --- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -79,7 +79,8 @@ /// 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 , llvm::StringRef Code, +std::string fixIncludes(const AnalysisResults , +llvm::StringRef FileName, llvm::StringRef Code, const format::FormatStyle ); /// Gets all the providers for a symbol by traversing each location. Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp === --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -274,7 +274,7 @@ } TEST(FixIncludes, Basic) { - llvm::StringRef Code = R"cpp( + llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" #include "b.h" #include @@ -300,7 +300,8 @@ Results.Unused.push_back(Inc.atLine(3)); Results.Unused.push_back(Inc.atLine(4)); - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()), +R"cpp(#include "d.h" #include "a.h" #include "aa.h" #include "ab.h" Index: