Skip to content

Commit

Permalink
Revert "[clang-format] SortIncludes should support "@import" lines in…
Browse files Browse the repository at this point in the history 
… 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.
  • Loading branch information
@krasimirgg
krasimirgg committed Apr 28, 2022
1 parent 57f99d0 commit e8cc749
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 158 deletions.
17 changes: 0 additions & 17 deletions clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
View file
Expand Up @@ -129,23 +129,6 @@ class HeaderIncludes {
llvm::Regex IncludeRegex;
};

/// \returns a regex that can match various styles of C++ includes.
/// For example:
/// \code
/// #include <foo.h>
/// @import 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

Expand Down
40 changes: 19 additions & 21 deletions clang/lib/Format/Format.cpp
View file
Expand Up @@ -44,7 +44,6 @@
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/YAMLTraits.h"
#include <algorithm>
#include <limits>
#include <memory>
#include <mutex>
#include <string>
Expand Down Expand Up @@ -2725,6 +2724,13 @@ static void sortCppIncludes(const FormatStyle &Style,
}
}

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,
Expand All @@ -2734,7 +2740,7 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
.StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
.Default(0);
unsigned SearchFrom = 0;
llvm::Regex IncludeRegex(tooling::getCppIncludeRegex());
llvm::Regex IncludeRegex(CppIncludeRegexPattern);
SmallVector<StringRef, 4> Matches;
SmallVector<IncludeDirective, 16> IncludesInBlock;

Expand Down Expand Up @@ -2790,14 +2796,7 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
bool MergeWithNextLine = Trimmed.endswith("\\");
if (!FormattingOff && !MergeWithNextLine) {
if (IncludeRegex.match(Line, &Matches)) {
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;
}

StringRef IncludeName = Matches[2];
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.
Expand All @@ -2810,10 +2809,8 @@ tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
int Category = Categories.getIncludePriority(
IncludeName,
/*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
int Priority = WithSemicolon ? std::numeric_limits<int>::max()
: Categories.getSortIncludePriority(
IncludeName, !MainIncludeFound &&
FirstIncludeBlock);
int Priority = Categories.getSortIncludePriority(
IncludeName, !MainIncludeFound && FirstIncludeBlock);
if (Category == 0)
MainIncludeFound = true;
IncludesInBlock.push_back(
Expand Down Expand Up @@ -3073,15 +3070,16 @@ namespace {

inline bool isHeaderInsertion(const tooling::Replacement &Replace) {
return Replace.getOffset() == UINT_MAX && Replace.getLength() == 0 &&
tooling::getCppIncludeRegex().match(Replace.getReplacementText());
llvm::Regex(CppIncludeRegexPattern)
.match(Replace.getReplacementText());
}

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

// FIXME: insert empty lines between newly created blocks.
static tooling::Replacements
tooling::Replacements
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
const FormatStyle &Style) {
if (!Style.isCpp())
Expand Down Expand Up @@ -3113,7 +3111,7 @@ fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,

for (const auto &Header : HeadersToDelete) {
tooling::Replacements Replaces =
Includes.remove(tooling::trimInclude(Header), Header.startswith("<"));
Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
for (const auto &R : Replaces) {
auto Err = Result.add(R);
if (Err) {
Expand All @@ -3125,17 +3123,17 @@ fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
}
}

llvm::Regex IncludeRegex = tooling::getCppIncludeRegex();
llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
llvm::SmallVector<StringRef, 4> Matches;
for (const auto &R : HeaderInsertions) {
auto IncludeDirective = R.getReplacementText();
bool Matched = IncludeRegex.match(IncludeDirective, &Matches);
assert(Matched && "Header insertion replacement must have replacement text "
"'#include ...'");
(void)Matched;
StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
auto Replace = Includes.insert(tooling::trimInclude(IncludeName),
IncludeName.startswith("<"));
auto IncludeName = Matches[2];
auto Replace =
Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
if (Replace) {
auto Err = Result.add(*Replace);
if (Err) {
Expand Down
33 changes: 10 additions & 23 deletions clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
View file
Expand Up @@ -169,6 +169,13 @@ unsigned getMaxHeaderInsertionOffset(StringRef FileName, StringRef Code,
});
}

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*
Expand Down Expand Up @@ -267,7 +274,8 @@ HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
MaxInsertOffset(MinInsertOffset +
getMaxHeaderInsertionOffset(
FileName, Code.drop_front(MinInsertOffset), Style)),
Categories(Style, FileName), IncludeRegex(getCppIncludeRegex()) {
Categories(Style, FileName),
IncludeRegex(llvm::Regex(IncludeRegexPattern)) {
// Add 0 for main header and INT_MAX for headers that are not in any
// category.
Priorities = {0, INT_MAX};
Expand All @@ -282,11 +290,10 @@ HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
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(IncludeName,
Include(Matches[2],
tooling::Range(
Offset, std::min(Line.size() + 1, Code.size() - Offset))),
NextLineOffset);
Expand Down Expand Up @@ -396,25 +403,5 @@ tooling::Replacements HeaderIncludes::remove(llvm::StringRef IncludeName,
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
97 changes: 0 additions & 97 deletions clang/unittests/Format/SortIncludesTest.cpp
View file
Expand Up @@ -458,103 +458,6 @@ TEST_F(SortIncludesTest, HandlesMultilineIncludes) {
"#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"
Expand Down

0 comments on commit e8cc749

Please sign in to comment.