kwk updated this revision to Diff 423256.
kwk added a comment.
- Use proposed reverse loop
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121370/new/
https://reviews.llvm.org/D121370
Files:
clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
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,103 @@
"#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 that shows main headers in two groups are still found and sorting
+ // still works. The @import's are kept in their respective group but are
+ // put at the end of each group.
+ FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
+ EXPECT_EQ("#import \"foo.hpp\"\n"
+ "#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Base;\n"
+ "@import Foundation;\n"
+ "@import foundation;\n"
+ "\n"
+ "#import \"foo.h\"\n"
+ "#include \"foobar\"\n"
+ "#import <f/g.h>\n"
+ "@import AAAA;\n"
+ "@import aaaa;\n",
+ sort("#import \"b.h\"\n"
+ "@import Foundation;\n"
+ "@import foundation;\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Base;\n"
+ "#import \"foo.hpp\"\n"
+ "\n"
+ "@import aaaa;\n"
+ "#import <f/g.h>\n"
+ "@import AAAA;\n"
+ "#include \"foobar\"\n"
+ "#import \"foo.h\"\n",
+ "foo.c", 2));
+
+ // Regrouping and putting @import's in the very last group
+ FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
+ EXPECT_EQ("#import \"foo.hpp\"\n"
+ "\n"
+ "#import \"b.h\"\n"
+ "#import \"c.h\"\n"
+ "#import \"foo.h\"\n"
+ "#include \"foobar\"\n"
+ "\n"
+ "#import <d/e.h>\n"
+ "#import <f/g.h>\n"
+ "\n"
+ "@import AAAA;\n"
+ "@import Base;\n"
+ "@import Foundation;\n"
+ "@import aaaa;\n"
+ "@import foundation;\n",
+ sort("#import \"b.h\"\n"
+ "@import Foundation;\n"
+ "@import foundation;\n"
+ "#import \"c.h\"\n"
+ "#import <d/e.h>\n"
+ "@import Base;\n"
+ "#import \"foo.hpp\"\n"
+ "\n"
+ "@import aaaa;\n"
+ "#import <f/g.h>\n"
+ "@import AAAA;\n"
+ "#include \"foobar\"\n"
+ "#import \"foo.h\"\n",
+ "foo.c"));
+}
+
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
@@ -169,13 +169,6 @@
});
}
-inline StringRef trimInclude(StringRef IncludeName) {
- return IncludeName.trim("\"<>");
-}
-
-const char IncludeRegexPattern[] =
- R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
// The filename of Path excluding extension.
// Used to match implementation with headers, this differs from sys::path::stem:
// - in names with multiple dots (foo.cu.cc) it terminates at the *first*
@@ -274,8 +267,7 @@
MaxInsertOffset(MinInsertOffset +
getMaxHeaderInsertionOffset(
FileName, Code.drop_front(MinInsertOffset), Style)),
- Categories(Style, FileName),
- IncludeRegex(llvm::Regex(IncludeRegexPattern)) {
+ Categories(Style, FileName), IncludeRegex(GetCppIncludeRegex()) {
// Add 0 for main header and INT_MAX for headers that are not in any
// category.
Priorities = {0, INT_MAX};
@@ -290,10 +282,11 @@
for (auto Line : Lines) {
NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
if (IncludeRegex.match(Line, &Matches)) {
+ StringRef IncludeName = tooling::GetIncludeNameFromMatches(Matches);
// 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);
@@ -325,7 +318,7 @@
void HeaderIncludes::addExistingInclude(Include IncludeToAdd,
unsigned NextLineOffset) {
auto Iter =
- ExistingIncludes.try_emplace(trimInclude(IncludeToAdd.Name)).first;
+ ExistingIncludes.try_emplace(TrimInclude(IncludeToAdd.Name)).first;
Iter->second.push_back(std::move(IncludeToAdd));
auto &CurInclude = Iter->second.back();
// The header name with quotes or angle brackets.
@@ -342,7 +335,7 @@
llvm::Optional<tooling::Replacement>
HeaderIncludes::insert(llvm::StringRef IncludeName, bool IsAngled) const {
- assert(IncludeName == trimInclude(IncludeName));
+ assert(IncludeName == TrimInclude(IncludeName));
// If a <header> ("header") already exists in code, "header" (<header>) with
// different quotation will still be inserted.
// FIXME: figure out if this is the best behavior.
@@ -383,7 +376,7 @@
tooling::Replacements HeaderIncludes::remove(llvm::StringRef IncludeName,
bool IsAngled) const {
- assert(IncludeName == trimInclude(IncludeName));
+ assert(IncludeName == TrimInclude(IncludeName));
tooling::Replacements Result;
auto Iter = ExistingIncludes.find(IncludeName);
if (Iter == ExistingIncludes.end())
@@ -403,5 +396,25 @@
return Result;
}
+llvm::Regex GetCppIncludeRegex() {
+ static const char CppIncludeRegexPattern[] =
+ R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
+ return llvm::Regex(CppIncludeRegexPattern);
+}
+
+llvm::StringRef GetIncludeNameFromMatches(
+ const llvm::SmallVectorImpl<llvm::StringRef> &Matches) {
+ for (auto Match : llvm::reverse(Matches)) {
+ if (!Match.empty())
+ return Match;
+ }
+ llvm_unreachable("No non-empty match group found in list of matches");
+ return llvm::StringRef();
+}
+
+llvm::StringRef TrimInclude(llvm::StringRef IncludeName) {
+ return IncludeName.trim("\"<>;");
+}
+
} // namespace tooling
} // namespace clang
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -44,6 +44,7 @@
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/YAMLTraits.h"
#include <algorithm>
+#include <limits>
#include <memory>
#include <mutex>
#include <string>
@@ -2721,13 +2722,6 @@
}
}
-namespace {
-
-const char CppIncludeRegexPattern[] =
- R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
-} // anonymous namespace
-
tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
ArrayRef<tooling::Range> Ranges,
StringRef FileName,
@@ -2737,7 +2731,7 @@
.StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
.Default(0);
unsigned SearchFrom = 0;
- llvm::Regex IncludeRegex(CppIncludeRegexPattern);
+ llvm::Regex IncludeRegex(tooling::GetCppIncludeRegex());
SmallVector<StringRef, 4> Matches;
SmallVector<IncludeDirective, 16> IncludesInBlock;
@@ -2793,7 +2787,14 @@
bool MergeWithNextLine = Trimmed.endswith("\\");
if (!FormattingOff && !MergeWithNextLine) {
if (IncludeRegex.match(Line, &Matches)) {
- StringRef IncludeName = Matches[2];
+ StringRef IncludeName = tooling::GetIncludeNameFromMatches(Matches);
+ // This addresses https://github.com/llvm/llvm-project/issues/38995
+ bool 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.
@@ -2806,8 +2807,10 @@
int Category = Categories.getIncludePriority(
IncludeName,
/*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
- int Priority = Categories.getSortIncludePriority(
- IncludeName, !MainIncludeFound && FirstIncludeBlock);
+ int Priority = WithSemicolon ? std::numeric_limits<int>::max()
+ : Categories.getSortIncludePriority(
+ IncludeName, !MainIncludeFound &&
+ FirstIncludeBlock);
if (Category == 0)
MainIncludeFound = true;
IncludesInBlock.push_back(
@@ -3067,8 +3070,7 @@
inline bool isHeaderInsertion(const tooling::Replacement &Replace) {
return Replace.getOffset() == UINT_MAX && Replace.getLength() == 0 &&
- llvm::Regex(CppIncludeRegexPattern)
- .match(Replace.getReplacementText());
+ tooling::GetCppIncludeRegex().match(Replace.getReplacementText());
}
inline bool isHeaderDeletion(const tooling::Replacement &Replace) {
@@ -3108,7 +3110,7 @@
for (const auto &Header : HeadersToDelete) {
tooling::Replacements Replaces =
- Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
+ Includes.remove(tooling::TrimInclude(Header), Header.startswith("<"));
for (const auto &R : Replaces) {
auto Err = Result.add(R);
if (Err) {
@@ -3120,7 +3122,7 @@
}
}
- llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
+ llvm::Regex IncludeRegex = tooling::GetCppIncludeRegex();
llvm::SmallVector<StringRef, 4> Matches;
for (const auto &R : HeaderInsertions) {
auto IncludeDirective = R.getReplacementText();
@@ -3128,9 +3130,9 @@
assert(Matched && "Header insertion replacement must have replacement text "
"'#include ...'");
(void)Matched;
- auto IncludeName = Matches[2];
- auto Replace =
- Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
+ StringRef IncludeName = tooling::GetIncludeNameFromMatches(Matches);
+ auto Replace = Includes.insert(tooling::TrimInclude(IncludeName),
+ IncludeName.startswith("<"));
if (Replace) {
auto Err = Result.add(*Replace);
if (Err) {
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===================================================================
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -129,6 +129,23 @@
llvm::Regex IncludeRegex;
};
+/// \returns a regex that can match various styles of C++ includes.
+/// For example:
+/// \code
+/// #include <foo.h>
+/// @include bar;
+/// #include "bar.h"
+/// \endcode
+llvm::Regex GetCppIncludeRegex();
+
+/// \returns the last match in the list of matches that is not empty.
+llvm::StringRef GetIncludeNameFromMatches(
+ const llvm::SmallVectorImpl<llvm::StringRef> &Matches);
+
+/// \returns the given include name and removes the following symbols from the
+/// beginning and ending of the include name: " > < ;
+llvm::StringRef TrimInclude(llvm::StringRef IncludeName);
+
} // namespace tooling
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
