Skip to content

Commit e8cc749

Browse files
krasimirggkrasimirgg
committed
Revert "[clang-format] SortIncludes should support "@import" lines in Objective-C"
This reverts commit d46fa02. Regressed include order in some cases with trailing comments, see the comments on https://reviews.llvm.org/D121370. Will add a regression test in a follow-up commit.
1 parent 57f99d0 commit e8cc749

File tree

4 files changed

+29
-158
lines changed

4 files changed

+29
-158
lines changed

‎clang/include/clang/Tooling/Inclusions/HeaderIncludes.h‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -129,23 +129,6 @@ class HeaderIncludes {
129129
llvm::Regex IncludeRegex;
130130
};
131131

132-
/// \returns a regex that can match various styles of C++ includes.
133-
/// For example:
134-
/// \code
135-
/// #include <foo.h>
136-
/// @import bar;
137-
/// #include "bar.h"
138-
/// \endcode
139-
llvm::Regex getCppIncludeRegex();
140-
141-
/// \returns the last match in the list of matches that is not empty.
142-
llvm::StringRef getIncludeNameFromMatches(
143-
const llvm::SmallVectorImpl<llvm::StringRef> &Matches);
144-
145-
/// \returns the given include name and removes the following symbols from the
146-
/// beginning and ending of the include name: " > < ;
147-
llvm::StringRef trimInclude(llvm::StringRef IncludeName);
148-
149132
} // namespace tooling
150133
} // namespace clang
151134

‎clang/lib/Format/Format.cpp‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
#include "llvm/Support/VirtualFileSystem.h"
4545
#include "llvm/Support/YAMLTraits.h"
4646
#include <algorithm>
47-
#include <limits>
4847
#include <memory>
4948
#include <mutex>
5049
#include <string>
@@ -2725,6 +2724,13 @@ static void sortCppIncludes(const FormatStyle &Style,
27252724
}
27262725
}
27272726

2727+
namespace {
2728+
2729+
const char CppIncludeRegexPattern[] =
2730+
R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
2731+
2732+
} // anonymous namespace
2733+
27282734
tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
27292735
ArrayRef<tooling::Range> Ranges,
27302736
StringRef FileName,
@@ -2734,7 +2740,7 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
27342740
.StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
27352741
.Default(0);
27362742
unsigned SearchFrom = 0;
2737-
llvm::Regex IncludeRegex(tooling::getCppIncludeRegex());
2743+
llvm::Regex IncludeRegex(CppIncludeRegexPattern);
27382744
SmallVector<StringRef, 4> Matches;
27392745
SmallVector<IncludeDirective, 16> IncludesInBlock;
27402746

@@ -2790,14 +2796,7 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
27902796
bool MergeWithNextLine = Trimmed.endswith("\\");
27912797
if (!FormattingOff && !MergeWithNextLine) {
27922798
if (IncludeRegex.match(Line, &Matches)) {
2793-
StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
2794-
// This addresses https://github.com/llvm/llvm-project/issues/38995
2795-
bool WithSemicolon = false;
2796-
if (!IncludeName.startswith("\"") && !IncludeName.startswith("<") &&
2797-
IncludeName.endswith(";")) {
2798-
WithSemicolon = true;
2799-
}
2800-
2799+
StringRef IncludeName = Matches[2];
28012800
if (Line.contains("/*") && !Line.contains("*/")) {
28022801
// #include with a start of a block comment, but without the end.
28032802
// Need to keep all the lines until the end of the comment together.
@@ -2810,10 +2809,8 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
28102809
int Category = Categories.getIncludePriority(
28112810
IncludeName,
28122811
/*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
2813-
int Priority = WithSemicolon ? std::numeric_limits<int>::max()
2814-
: Categories.getSortIncludePriority(
2815-
IncludeName, !MainIncludeFound &&
2816-
FirstIncludeBlock);
2812+
int Priority = Categories.getSortIncludePriority(
2813+
IncludeName, !MainIncludeFound && FirstIncludeBlock);
28172814
if (Category == 0)
28182815
MainIncludeFound = true;
28192816
IncludesInBlock.push_back(
@@ -3073,15 +3070,16 @@ namespace {
30733070

30743071
inline bool isHeaderInsertion(const tooling::Replacement &Replace) {
30753072
return Replace.getOffset() == UINT_MAX && Replace.getLength() == 0 &&
3076-
tooling::getCppIncludeRegex().match(Replace.getReplacementText());
3073+
llvm::Regex(CppIncludeRegexPattern)
3074+
.match(Replace.getReplacementText());
30773075
}
30783076

30793077
inline bool isHeaderDeletion(const tooling::Replacement &Replace) {
30803078
return Replace.getOffset() == UINT_MAX && Replace.getLength() == 1;
30813079
}
30823080

30833081
// FIXME: insert empty lines between newly created blocks.
3084-
static tooling::Replacements
3082+
tooling::Replacements
30853083
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
30863084
const FormatStyle &Style) {
30873085
if (!Style.isCpp())
@@ -3113,7 +3111,7 @@ fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
31133111

31143112
for (const auto &Header : HeadersToDelete) {
31153113
tooling::Replacements Replaces =
3116-
Includes.remove(tooling::trimInclude(Header), Header.startswith("<"));
3114+
Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
31173115
for (const auto &R : Replaces) {
31183116
auto Err = Result.add(R);
31193117
if (Err) {
@@ -3125,17 +3123,17 @@ fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
31253123
}
31263124
}
31273125

3128-
llvm::Regex IncludeRegex = tooling::getCppIncludeRegex();
3126+
llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
31293127
llvm::SmallVector<StringRef, 4> Matches;
31303128
for (const auto &R : HeaderInsertions) {
31313129
auto IncludeDirective = R.getReplacementText();
31323130
bool Matched = IncludeRegex.match(IncludeDirective, &Matches);
31333131
assert(Matched && "Header insertion replacement must have replacement text "
31343132
"'#include ...'");
31353133
(void)Matched;
3136-
StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
3137-
auto Replace = Includes.insert(tooling::trimInclude(IncludeName),
3138-
IncludeName.startswith("<"));
3134+
auto IncludeName = Matches[2];
3135+
auto Replace =
3136+
Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
31393137
if (Replace) {
31403138
auto Err = Result.add(*Replace);
31413139
if (Err) {

‎clang/lib/Tooling/Inclusions/HeaderIncludes.cpp‎

Lines changed: 10 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,13 @@ unsigned getMaxHeaderInsertionOffset(StringRef FileName, StringRef Code,
169169
});
170170
}
171171

172+
inline StringRef trimInclude(StringRef IncludeName) {
173+
return IncludeName.trim("\"<>");
174+
}
175+
176+
const char IncludeRegexPattern[] =
177+
R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
178+
172179
// The filename of Path excluding extension.
173180
// Used to match implementation with headers, this differs from sys::path::stem:
174181
// - in names with multiple dots (foo.cu.cc) it terminates at the *first*
@@ -267,7 +274,8 @@ HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
267274
MaxInsertOffset(MinInsertOffset +
268275
getMaxHeaderInsertionOffset(
269276
FileName, Code.drop_front(MinInsertOffset), Style)),
270-
Categories(Style, FileName), IncludeRegex(getCppIncludeRegex()) {
277+
Categories(Style, FileName),
278+
IncludeRegex(llvm::Regex(IncludeRegexPattern)) {
271279
// Add 0 for main header and INT_MAX for headers that are not in any
272280
// category.
273281
Priorities = {0, INT_MAX};
@@ -282,11 +290,10 @@ HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
282290
for (auto Line : Lines) {
283291
NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
284292
if (IncludeRegex.match(Line, &Matches)) {
285-
StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
286293
// If this is the last line without trailing newline, we need to make
287294
// sure we don't delete across the file boundary.
288295
addExistingInclude(
289-
Include(IncludeName,
296+
Include(Matches[2],
290297
tooling::Range(
291298
Offset, std::min(Line.size() + 1, Code.size() - Offset))),
292299
NextLineOffset);
@@ -396,25 +403,5 @@ tooling::Replacements HeaderIncludes::remove(llvm::StringRef IncludeName,
396403
return Result;
397404
}
398405

399-
llvm::Regex getCppIncludeRegex() {
400-
static const char CppIncludeRegexPattern[] =
401-
R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
402-
return llvm::Regex(CppIncludeRegexPattern);
403-
}
404-
405-
llvm::StringRef getIncludeNameFromMatches(
406-
const llvm::SmallVectorImpl<llvm::StringRef> &Matches) {
407-
for (auto Match : llvm::reverse(Matches)) {
408-
if (!Match.empty())
409-
return Match;
410-
}
411-
llvm_unreachable("No non-empty match group found in list of matches");
412-
return llvm::StringRef();
413-
}
414-
415-
llvm::StringRef trimInclude(llvm::StringRef IncludeName) {
416-
return IncludeName.trim("\"<>;");
417-
}
418-
419406
} // namespace tooling
420407
} // namespace clang

‎clang/unittests/Format/SortIncludesTest.cpp‎

Lines changed: 0 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -458,103 +458,6 @@ TEST_F(SortIncludesTest, HandlesMultilineIncludes) {
458458
"#include \"b.h\"\n"));
459459
}
460460

461-
TEST_F(SortIncludesTest, SupportAtImportLines) {
462-
// Test from https://github.com/llvm/llvm-project/issues/38995
463-
EXPECT_EQ("#import \"a.h\"\n"
464-
"#import \"b.h\"\n"
465-
"#import \"c.h\"\n"
466-
"#import <d/e.h>\n"
467-
"@import Foundation;\n",
468-
sort("#import \"b.h\"\n"
469-
"#import \"c.h\"\n"
470-
"#import <d/e.h>\n"
471-
"@import Foundation;\n"
472-
"#import \"a.h\"\n"));
473-
474-
// Slightly more complicated test that shows sorting in each priorities still
475-
// works.
476-
EXPECT_EQ("#import \"a.h\"\n"
477-
"#import \"b.h\"\n"
478-
"#import \"c.h\"\n"
479-
"#import <d/e.h>\n"
480-
"@import Base;\n"
481-
"@import Foundation;\n"
482-
"@import base;\n"
483-
"@import foundation;\n",
484-
sort("#import \"b.h\"\n"
485-
"#import \"c.h\"\n"
486-
"@import Base;\n"
487-
"#import <d/e.h>\n"
488-
"@import foundation;\n"
489-
"@import Foundation;\n"
490-
"@import base;\n"
491-
"#import \"a.h\"\n"));
492-
493-
// Test that shows main headers in two groups are still found and sorting
494-
// still works. The @import's are kept in their respective group but are
495-
// put at the end of each group.
496-
FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
497-
EXPECT_EQ("#import \"foo.hpp\"\n"
498-
"#import \"b.h\"\n"
499-
"#import \"c.h\"\n"
500-
"#import <d/e.h>\n"
501-
"@import Base;\n"
502-
"@import Foundation;\n"
503-
"@import foundation;\n"
504-
"\n"
505-
"#import \"foo.h\"\n"
506-
"#include \"foobar\"\n"
507-
"#import <f/g.h>\n"
508-
"@import AAAA;\n"
509-
"@import aaaa;\n",
510-
sort("#import \"b.h\"\n"
511-
"@import Foundation;\n"
512-
"@import foundation;\n"
513-
"#import \"c.h\"\n"
514-
"#import <d/e.h>\n"
515-
"@import Base;\n"
516-
"#import \"foo.hpp\"\n"
517-
"\n"
518-
"@import aaaa;\n"
519-
"#import <f/g.h>\n"
520-
"@import AAAA;\n"
521-
"#include \"foobar\"\n"
522-
"#import \"foo.h\"\n",
523-
"foo.c", 2));
524-
525-
// Regrouping and putting @import's in the very last group
526-
FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
527-
EXPECT_EQ("#import \"foo.hpp\"\n"
528-
"\n"
529-
"#import \"b.h\"\n"
530-
"#import \"c.h\"\n"
531-
"#import \"foo.h\"\n"
532-
"#include \"foobar\"\n"
533-
"\n"
534-
"#import <d/e.h>\n"
535-
"#import <f/g.h>\n"
536-
"\n"
537-
"@import AAAA;\n"
538-
"@import Base;\n"
539-
"@import Foundation;\n"
540-
"@import aaaa;\n"
541-
"@import foundation;\n",
542-
sort("#import \"b.h\"\n"
543-
"@import Foundation;\n"
544-
"@import foundation;\n"
545-
"#import \"c.h\"\n"
546-
"#import <d/e.h>\n"
547-
"@import Base;\n"
548-
"#import \"foo.hpp\"\n"
549-
"\n"
550-
"@import aaaa;\n"
551-
"#import <f/g.h>\n"
552-
"@import AAAA;\n"
553-
"#include \"foobar\"\n"
554-
"#import \"foo.h\"\n",
555-
"foo.c"));
556-
}
557-
558461
TEST_F(SortIncludesTest, LeavesMainHeaderFirst) {
559462
Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
560463
EXPECT_EQ("#include \"llvm/a.h\"\n"

0 commit comments

Comments
 (0)