kwk updated this revision to Diff 415487.
kwk added a comment.
Changed the strategy of how includes are sorted by increasing the priority if
an include is of the "new" type: `@import Foundation;`. This will sort these
includes after everything else just as requested in
https://github.com/llvm/llvm-project/issues/38995.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121370/new/
https://reviews.llvm.org/D121370
Files:
clang/lib/Format/Format.cpp
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
clang/unittests/Format/SortIncludesTest.cpp
Index: clang/unittests/Format/SortIncludesTest.cpp
===================================================================
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -458,6 +458,39 @@
"#include \"b.h\"\n"));
}
+TEST_F(SortIncludesTest, SupportAtImportLines) {
+ // Test from https://github.com/llvm/llvm-project/issues/38995
+ EXPECT_EQ("#import \"a.h\"\n"
+ "#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Foundation;\n",
+ sort("#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Foundation;\n"
+ "#import \"a.h\"\n"));
+
+ // Slightly more complicated test that shows sorting in each priorities still
+ // works.
+ EXPECT_EQ("#import \"a.h\"\n"
+ "#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Base;\n"
+ "@import Foundation;\n"
+ "@import base;\n"
+ "@import foundation;\n",
+ sort("#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "@import Base;\n"
+ "#import <d/e.h>\n"
+ "@import foundation;\n"
+ "@import Foundation;\n"
+ "@import base;\n"
+ "#import \"a.h\"\n"));
+}
+
TEST_F(SortIncludesTest, LeavesMainHeaderFirst) {
Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
EXPECT_EQ("#include \"llvm/a.h\"\n"
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -170,11 +170,11 @@
}
inline StringRef trimInclude(StringRef IncludeName) {
- return IncludeName.trim("\"<>");
+ return IncludeName.trim("\"<>;");
}
const char IncludeRegexPattern[] =
- R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+ R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
// The filename of Path excluding extension.
// Used to match implementation with headers, this differs from sys::path::stem:
@@ -290,10 +290,17 @@
for (auto Line : Lines) {
NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
if (IncludeRegex.match(Line, &Matches)) {
+ StringRef IncludeName;
+ for (int i=Matches.size()-1; i>0; i--) {
+ if (!Matches[i].empty()) {
+ IncludeName = Matches[i];
+ break;
+ }
+ }
// If this is the last line without trailing newline, we need to make
// sure we don't delete across the file boundary.
addExistingInclude(
- Include(Matches[2],
+ Include(IncludeName,
tooling::Range(
Offset, std::min(Line.size() + 1, Code.size() - Offset))),
NextLineOffset);
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2682,8 +2682,7 @@
namespace {
const char CppIncludeRegexPattern[] =
- R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
+ R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
} // anonymous namespace
tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
@@ -2698,7 +2697,7 @@
llvm::Regex IncludeRegex(CppIncludeRegexPattern);
SmallVector<StringRef, 4> Matches;
SmallVector<IncludeDirective, 16> IncludesInBlock;
-
+
// In compiled files, consider the first #include to be the main #include of
// the file if it is not a system #include. This ensures that the header
// doesn't have hidden dependencies
@@ -2751,7 +2750,19 @@
bool MergeWithNextLine = Trimmed.endswith("\\");
if (!FormattingOff && !MergeWithNextLine) {
if (IncludeRegex.match(Line, &Matches)) {
- StringRef IncludeName = Matches[2];
+ StringRef IncludeName;
+ for (int i=Matches.size()-1; i>0; i--) {
+ if (!Matches[i].empty()) {
+ IncludeName = Matches[i];
+ break;
+ }
+ }
+ // This addresses https://github.com/llvm/llvm-project/issues/38995
+ int WithSemicolon = false;
+ if (!IncludeName.startswith("\"") && !IncludeName.startswith("<") && IncludeName.endswith(";")) {
+ WithSemicolon = true;
+ }
+
if (Line.contains("/*") && !Line.contains("*/")) {
// #include with a start of a block comment, but without the end.
// Need to keep all the lines until the end of the comment together.
@@ -2764,7 +2775,7 @@
int Category = Categories.getIncludePriority(
IncludeName,
/*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
- int Priority = Categories.getSortIncludePriority(
+ int Priority = WithSemicolon ? INT_MAX : Categories.getSortIncludePriority(
IncludeName, !MainIncludeFound && FirstIncludeBlock);
if (Category == 0)
MainIncludeFound = true;
@@ -3033,6 +3044,10 @@
return Replace.getOffset() == UINT_MAX && Replace.getLength() == 1;
}
+inline StringRef trimInclude(StringRef IncludeName) {
+ return IncludeName.trim("\"<>;");
+}
+
// FIXME: insert empty lines between newly created blocks.
tooling::Replacements
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
@@ -3066,7 +3081,7 @@
for (const auto &Header : HeadersToDelete) {
tooling::Replacements Replaces =
- Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
+ Includes.remove(trimInclude(Header), Header.startswith("<"));
for (const auto &R : Replaces) {
auto Err = Result.add(R);
if (Err) {
@@ -3086,9 +3101,16 @@
assert(Matched && "Header insertion replacement must have replacement text "
"'#include ...'");
(void)Matched;
- auto IncludeName = Matches[2];
+ StringRef IncludeName;
+ for (int i=Matches.size()-1; i>0; i--) {
+ if (!Matches[i].empty()) {
+ IncludeName = Matches[i];
+ break;
+ }
+ }
+ // auto IncludeName = Matches[2];
auto Replace =
- Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
+ Includes.insert(trimInclude(IncludeName), IncludeName.startswith("<"));
if (Replace) {
auto Err = Result.add(*Replace);
if (Err) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
