This is an archive of the discontinued LLVM Phabricator instance.

Differential D157568

[clang-format] Handle NamespaceMacro string arg for FixNamespaceComments
ClosedPublic

Authored by owenpan on Aug 9 2023, 6:11 PM.

Details

Reviewers
rymiel
HazardyKnusperkeks
MyDeveloperDay
Commits
rG063c42e919c0: [clang-format] Handle NamespaceMacro string arg for FixNamespaceComments
Summary

Fixes https://github.com/llvm/llvm-project/issues/63795.

Diff Detail

Event Timeline

owenpan created this revision.Aug 9 2023, 6:11 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptAug 9 2023, 6:11 PM
Herald added reviewers: rymiel, HazardyKnusperkeks, MyDeveloperDay. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
owenpan requested review of this revision.Aug 9 2023, 6:11 PM
owenpan updated this revision to Diff 548878.Aug 9 2023, 10:27 PM

Rebased and updated test cases.

Harbormaster completed remote builds in B251577: Diff 548878.Aug 10 2023, 2:52 AM
HazardyKnusperkeks accepted this revision.Aug 13 2023, 11:46 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/NamespaceEndCommentsFixer.cpp
177

Maybe this?

This revision is now accepted and ready to land.Aug 13 2023, 11:46 AM
owenpan added inline comments.Aug 13 2023, 1:30 PM
clang/lib/Format/NamespaceEndCommentsFixer.cpp
177

I don't think we'd need it because the + is greedy.

Closed by commit rG063c42e919c0: [clang-format] Handle NamespaceMacro string arg for FixNamespaceComments (authored by owenpan). · Explain WhyAug 16 2023, 10:46 AM
This revision was automatically updated to reflect the committed changes.
owenpan added a commit: rG063c42e919c0: [clang-format] Handle NamespaceMacro string arg for FixNamespaceComments.

Revision Contents

PathSize
clang/
lib/
Format/
2 lines
unittests/
Format/
82 lines

Diff 550807

clang/lib/Format/NamespaceEndCommentsFixer.cpp

Show First 20 Lines • Show All 168 Lines • ▼ Show 20 Linesbool validEndComment(const FormatToken *RBraceTok, StringRef NamespaceName,
// Matches a valid namespace end comment. // Matches a valid namespace end comment.
// Valid namespace end comments don't need to be edited. // Valid namespace end comments don't need to be edited.
static const llvm::Regex NamespaceCommentPattern = static const llvm::Regex NamespaceCommentPattern =
llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *" llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
"namespace( +([a-zA-Z0-9:_]+))?\\.? *(\\*/)?$", "namespace( +([a-zA-Z0-9:_]+))?\\.? *(\\*/)?$",
llvm::Regex::IgnoreCase); llvm::Regex::IgnoreCase);
static const llvm::Regex NamespaceMacroCommentPattern = static const llvm::Regex NamespaceMacroCommentPattern =
llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *" llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
"([a-zA-Z0-9_]+)\\(([a-zA-Z0-9:_]*)\\)\\.? *(\\*/)?$", "([a-zA-Z0-9_]+)\\(([a-zA-Z0-9:_]*|\".+\")\\)\\.? *(\\*/)?$",
HazardyKnusperkeksUnsubmitted
Not Done
ReplyInline Actions
llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
- "([a-zA-Z0-9_]+)\\(([a-zA-Z0-9:_]*|\".+\")\\)\\.? *(\\*/)?$",
+ "([a-zA-Z0-9_]+)\\(([a-zA-Z0-9:_]*|\"[\"]+\")\\)\\.? *(\\*/)?$",
llvm::Regex::IgnoreCase);

Maybe this?

HazardyKnusperkeks: Maybe this?
owenpanAuthorUnsubmitted
Done
ReplyInline Actions

I don't think we'd need it because the + is greedy.

owenpan: I don't think we'd need it because the `+` is greedy.
llvm::Regex::IgnoreCase); llvm::Regex::IgnoreCase);
SmallVector<StringRef, 8> Groups; SmallVector<StringRef, 8> Groups;
if (NamespaceTok->is(TT_NamespaceMacro) && if (NamespaceTok->is(TT_NamespaceMacro) &&
NamespaceMacroCommentPattern.match(Comment->TokenText, &Groups)) { NamespaceMacroCommentPattern.match(Comment->TokenText, &Groups)) {
StringRef NamespaceTokenText = Groups.size() > 4 ? Groups[4] : ""; StringRef NamespaceTokenText = Groups.size() > 4 ? Groups[4] : "";
// The name of the macro must be used. // The name of the macro must be used.
if (NamespaceTokenText != NamespaceTok->TokenText) if (NamespaceTokenText != NamespaceTok->TokenText)
▲ Show 20 Lines • Show All 196 Lines • Show Last 20 Lines

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Show All 34 Linesprotected:
std::string std::string
fixNamespaceEndComments(llvm::StringRef Code, fixNamespaceEndComments(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle()) { const FormatStyle &Style = getLLVMStyle()) {
return fixNamespaceEndComments( return fixNamespaceEndComments(
Code, Code,
/*Ranges=*/{1, tooling::Range(0, Code.size())}, Style); /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
} }
bool isFormatted(StringRef Code, const std::vector<tooling::Range> &Ranges,
const FormatStyle &Style = getLLVMStyle()) const {
return clang::format::fixNamespaceEndComments(Style, Code, Ranges,
"<stdin>")
.empty();
}
bool isFormatted(StringRef Code,
const FormatStyle &Style = getLLVMStyle()) const {
return isFormatted(Code, {1, tooling::Range(0, Code.size())}, Style);
}
}; };
TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) { TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
EXPECT_EQ("namespace {\n" EXPECT_EQ("namespace {\n"
"int i;\n" "int i;\n"
"int j;\n" "int j;\n"
"} // namespace", "} // namespace",
fixNamespaceEndComments("namespace {\n" fixNamespaceEndComments("namespace {\n"
▲ Show 20 Lines • Show All 632 Lines • ▼ Show 20 Lines EXPECT_EQ("namespace {\n"
"int i;\n" "int i;\n"
"}; /* unnamed namespace */")); "}; /* unnamed namespace */"));
} }
TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) { TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
FormatStyle Style = getLLVMStyle(); FormatStyle Style = getLLVMStyle();
Style.NamespaceMacros.push_back("TESTSUITE"); Style.NamespaceMacros.push_back("TESTSUITE");
EXPECT_EQ("TESTSUITE() {\n" EXPECT_TRUE(isFormatted("TESTSUITE() {\n"
"int i;\n"
"} // end anonymous TESTSUITE()",
fixNamespaceEndComments("TESTSUITE() {\n"
"int i;\n" "int i;\n"
"} // end anonymous TESTSUITE()", "} // end anonymous TESTSUITE()",
Style)); Style));
EXPECT_EQ("TESTSUITE(A) {\n" EXPECT_TRUE(isFormatted("TESTSUITE(A) {\n"
"int i;\n"
"} /* end of TESTSUITE(A) */",
fixNamespaceEndComments("TESTSUITE(A) {\n"
"int i;\n" "int i;\n"
"} /* end of TESTSUITE(A) */", "} /* end of TESTSUITE(A) */",
Style)); Style));
EXPECT_EQ("TESTSUITE(A) {\n" EXPECT_TRUE(isFormatted("TESTSUITE(A) {\n"
"int i;\n"
"} // TESTSUITE(A)",
fixNamespaceEndComments("TESTSUITE(A) {\n"
"int i;\n" "int i;\n"
"} // TESTSUITE(A)", "} // TESTSUITE(A)",
Style)); Style));
EXPECT_EQ("TESTSUITE(A::B) {\n" EXPECT_TRUE(isFormatted("TESTSUITE(A::B) {\n"
"int i;\n"
"} // end TESTSUITE(A::B)",
fixNamespaceEndComments("TESTSUITE(A::B) {\n"
"int i;\n" "int i;\n"
"} // end TESTSUITE(A::B)", "} // end TESTSUITE(A::B)",
Style)); Style));
EXPECT_EQ("TESTSUITE(A) {\n" EXPECT_TRUE(isFormatted("TESTSUITE(A) {\n"
"int i;\n"
"}; // end TESTSUITE(A)",
fixNamespaceEndComments("TESTSUITE(A) {\n"
"int i;\n" "int i;\n"
"}; // end TESTSUITE(A)", "}; // end TESTSUITE(A)",
Style)); Style));
EXPECT_EQ("TESTSUITE() {\n" EXPECT_TRUE(isFormatted("TESTSUITE() {\n"
"int i;\n" "int i;\n"
"}; /* unnamed TESTSUITE() */", "}; /* unnamed TESTSUITE() */",
fixNamespaceEndComments("TESTSUITE() {\n" Style));
EXPECT_TRUE(isFormatted("TESTSUITE(\"foo\") {\n"
"int i;\n" "int i;\n"
"}; /* unnamed TESTSUITE() */", "} // TESTSUITE(\"foo\")",
Style)); Style));
} }
TEST_F(NamespaceEndCommentsFixerTest, UpdatesInvalidEndLineComment) { TEST_F(NamespaceEndCommentsFixerTest, UpdatesInvalidEndLineComment) {
EXPECT_EQ("namespace {\n" EXPECT_EQ("namespace {\n"
"int i;\n" "int i;\n"
"} // namespace", "} // namespace",
fixNamespaceEndComments("namespace {\n" fixNamespaceEndComments("namespace {\n"
"int i;\n" "int i;\n"
▲ Show 20 Lines • Show All 734 Lines • Show Last 20 Lines