[clang] d81f003 - [clang-format] Fix formatting of struct-like records followed by variable declaration.
Marek Kurdej via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 16 13:37:41 PST 2022
Author: Marek Kurdej
Date: 2022-02-16T22:37:32+01:00
New Revision: d81f003ce1419aee6cfb8d26f0bca9153278872a
URL: https://github.com/llvm/llvm-project/commit/d81f003ce1419aee6cfb8d26f0bca9153278872a
DIFF: https://github.com/llvm/llvm-project/commit/d81f003ce1419aee6cfb8d26f0bca9153278872a.diff
LOG: [clang-format] Fix formatting of struct-like records followed by variable declaration.
Fixes https://github.com/llvm/llvm-project/issues/24781.
Fixes https://github.com/llvm/llvm-project/issues/38160.
This patch splits `TT_RecordLBrace` for classes/enums/structs/unions (and other records, e.g. interfaces) and uses the brace type to avoid the error-prone scanning for record token.
The mentioned bugs were provoked by the scanning being too limited (and so not considering `const` or `constexpr`, or other qualifiers, on an anonymous struct variable declaration).
Moreover, the proposed solution is more efficient as we parse tokens once only (scanning being parsing too).
Reviewed By: MyDeveloperDay, HazardyKnusperkeks
Differential Revision: https://reviews.llvm.org/D119785
Added:
Modified:
clang/lib/Format/FormatToken.h
clang/lib/Format/TokenAnnotator.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/TokenAnnotatorTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index fee365ecc8f91..6aaf66c7bb7e5 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -37,6 +37,7 @@ namespace format {
TYPE(BlockComment) \
TYPE(BracedListLBrace) \
TYPE(CastRParen) \
+ TYPE(ClassLBrace) \
TYPE(CompoundRequirementLBrace) \
TYPE(ConditionalExpr) \
TYPE(ConflictAlternative) \
@@ -47,6 +48,7 @@ namespace format {
TYPE(DesignatedInitializerLSquare) \
TYPE(DesignatedInitializerPeriod) \
TYPE(DictLiteral) \
+ TYPE(EnumLBrace) \
TYPE(FatArrow) \
TYPE(ForEachMacro) \
TYPE(FunctionAnnotationRParen) \
@@ -108,6 +110,7 @@ namespace format {
TYPE(StartOfName) \
TYPE(StatementAttributeLikeMacro) \
TYPE(StatementMacro) \
+ TYPE(StructLBrace) \
TYPE(StructuredBindingLSquare) \
TYPE(TemplateCloser) \
TYPE(TemplateOpener) \
@@ -119,6 +122,7 @@ namespace format {
TYPE(TypeDeclarationParen) \
TYPE(TypenameMacro) \
TYPE(UnaryOperator) \
+ TYPE(UnionLBrace) \
TYPE(UntouchableMacroFunc) \
TYPE(CSharpStringLiteral) \
TYPE(CSharpNamedArgumentColon) \
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index f3885a1f683bc..206fa4541217c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1425,11 +1425,12 @@ class AnnotatingParser {
TT_LambdaArrow, TT_NamespaceMacro, TT_OverloadedOperator,
TT_RegexLiteral, TT_TemplateString, TT_ObjCStringLiteral,
TT_UntouchableMacroFunc, TT_StatementAttributeLikeMacro,
- TT_FunctionLikeOrFreestandingMacro, TT_RecordLBrace,
- TT_RequiresClause, TT_RequiresClauseInARequiresExpression,
- TT_RequiresExpression, TT_RequiresExpressionLParen,
- TT_RequiresExpressionLBrace, TT_BinaryOperator,
- TT_CompoundRequirementLBrace, TT_BracedListLBrace))
+ TT_FunctionLikeOrFreestandingMacro, TT_ClassLBrace, TT_EnumLBrace,
+ TT_RecordLBrace, TT_StructLBrace, TT_UnionLBrace, TT_RequiresClause,
+ TT_RequiresClauseInARequiresExpression, TT_RequiresExpression,
+ TT_RequiresExpressionLParen, TT_RequiresExpressionLBrace,
+ TT_BinaryOperator, TT_CompoundRequirementLBrace,
+ TT_BracedListLBrace))
CurrentToken->setType(TT_Unknown);
CurrentToken->Role.reset();
CurrentToken->MatchingParen = nullptr;
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 883030cb3dc16..16fa2e7b50f1c 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -26,6 +26,11 @@ bool startsExternCBlock(const AnnotatedLine &Line) {
NextNext && NextNext->is(tok::l_brace);
}
+bool isRecordLBrace(const FormatToken &Tok) {
+ return Tok.isOneOf(TT_ClassLBrace, TT_EnumLBrace, TT_RecordLBrace,
+ TT_StructLBrace, TT_UnionLBrace);
+}
+
/// Tracks the indent level of \c AnnotatedLines across levels.
///
/// \c nextLine must be called for each \c AnnotatedLine, after which \c
@@ -310,7 +315,7 @@ class LineJoiner {
for (const FormatToken *RecordTok = (*J)->Last; RecordTok;
RecordTok = RecordTok->Previous)
if (RecordTok->is(tok::l_brace))
- return RecordTok->is(TT_RecordLBrace);
+ return isRecordLBrace(*RecordTok);
}
}
@@ -457,27 +462,30 @@ class LineJoiner {
}
}
- // Try to merge a block with left brace unwrapped that wasn't yet covered.
if (TheLine->Last->is(tok::l_brace)) {
- const FormatToken *Tok = TheLine->First;
bool ShouldMerge = false;
- if (Tok->is(tok::kw_typedef)) {
- Tok = Tok->getNextNonComment();
- assert(Tok);
- }
- if (Tok->isOneOf(tok::kw_class, tok::kw_struct)) {
+ // Try to merge records.
+ if (TheLine->Last->is(TT_EnumLBrace)) {
+ ShouldMerge = Style.AllowShortEnumsOnASingleLine;
+ } else if (TheLine->Last->isOneOf(TT_ClassLBrace, TT_StructLBrace)) {
+ // NOTE: We use AfterClass (whereas AfterStruct exists) for both classes
+ // and structs, but it seems that wrapping is still handled correctly
+ // elsewhere.
ShouldMerge = !Style.BraceWrapping.AfterClass ||
(NextLine.First->is(tok::r_brace) &&
!Style.BraceWrapping.SplitEmptyRecord);
- } else if (Tok->is(tok::kw_enum)) {
- ShouldMerge = Style.AllowShortEnumsOnASingleLine;
} else {
+ // Try to merge a block with left brace unwrapped that wasn't yet
+ // covered.
+ assert(!TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
+ tok::kw_struct));
ShouldMerge = !Style.BraceWrapping.AfterFunction ||
(NextLine.First->is(tok::r_brace) &&
!Style.BraceWrapping.SplitEmptyFunction);
}
return ShouldMerge ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
+
// Try to merge a function block with left brace wrapped.
if (NextLine.First->is(TT_FunctionLBrace) &&
Style.BraceWrapping.AfterFunction) {
@@ -715,6 +723,7 @@ class LineJoiner {
const FormatToken *Next = Tok->getNextNonComment();
return !Next || Next->is(tok::semi);
};
+
if (ShouldMerge()) {
// We merge empty blocks even if the line exceeds the column limit.
Tok->SpacesRequiredBefore = Style.SpaceInEmptyBlock ? 1 : 0;
@@ -723,18 +732,7 @@ class LineJoiner {
} else if (Limit != 0 && !Line.startsWithNamespace() &&
!startsExternCBlock(Line)) {
// We don't merge short records.
- FormatToken *RecordTok = Line.First;
- // Skip record modifiers.
- while (RecordTok->Next &&
- RecordTok->isOneOf(tok::kw_typedef, tok::kw_export,
- Keywords.kw_declare, Keywords.kw_abstract,
- tok::kw_default, Keywords.kw_override,
- tok::kw_public, tok::kw_private,
- tok::kw_protected, Keywords.kw_internal))
- RecordTok = RecordTok->Next;
- if (RecordTok &&
- RecordTok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
- Keywords.kw_interface))
+ if (isRecordLBrace(*Line.Last))
return 0;
// Check that we still have three lines and they fit into the limit.
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 25db8ee2e7549..c1bd45beb7b37 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -3160,7 +3160,7 @@ bool UnwrappedLineParser::parseEnum() {
// Just a declaration or something is wrong.
if (FormatTok->isNot(tok::l_brace))
return true;
- FormatTok->setType(TT_RecordLBrace);
+ FormatTok->setType(TT_EnumLBrace);
FormatTok->setBlockKind(BK_Block);
if (Style.Language == FormatStyle::LK_Java) {
@@ -3397,8 +3397,22 @@ void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
nextToken();
}
}
+
+ auto GetBraceType = [](const FormatToken &RecordTok) {
+ switch (RecordTok.Tok.getKind()) {
+ case tok::kw_class:
+ return TT_ClassLBrace;
+ case tok::kw_struct:
+ return TT_StructLBrace;
+ case tok::kw_union:
+ return TT_UnionLBrace;
+ default:
+ // Useful for e.g. interface.
+ return TT_RecordLBrace;
+ }
+ };
if (FormatTok->Tok.is(tok::l_brace)) {
- FormatTok->setType(TT_RecordLBrace);
+ FormatTok->setType(GetBraceType(InitialToken));
if (ParseAsExpr) {
parseChildBlock();
} else {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 268fe16350b6b..6e5dd3284633c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -3395,10 +3395,43 @@ TEST_F(FormatTest, BreakInheritanceStyle) {
StyleWithInheritanceBreakAfterComma);
}
-TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
+TEST_F(FormatTest, FormatsVariableDeclarationsAfterRecord) {
verifyFormat("class A {\n} a, b;");
verifyFormat("struct A {\n} a, b;");
- verifyFormat("union A {\n} a;");
+ verifyFormat("union A {\n} a, b;");
+
+ verifyFormat("constexpr class A {\n} a, b;");
+ verifyFormat("constexpr struct A {\n} a, b;");
+ verifyFormat("constexpr union A {\n} a, b;");
+
+ verifyFormat("namespace {\nclass A {\n} a, b;\n} // namespace");
+ verifyFormat("namespace {\nstruct A {\n} a, b;\n} // namespace");
+ verifyFormat("namespace {\nunion A {\n} a, b;\n} // namespace");
+
+ verifyFormat("namespace {\nconstexpr class A {\n} a, b;\n} // namespace");
+ verifyFormat("namespace {\nconstexpr struct A {\n} a, b;\n} // namespace");
+ verifyFormat("namespace {\nconstexpr union A {\n} a, b;\n} // namespace");
+
+ verifyFormat("namespace ns {\n"
+ "class {\n"
+ "} a, b;\n"
+ "} // namespace ns");
+ verifyFormat("namespace ns {\n"
+ "const class {\n"
+ "} a, b;\n"
+ "} // namespace ns");
+ verifyFormat("namespace ns {\n"
+ "constexpr class C {\n"
+ "} a, b;\n"
+ "} // namespace ns");
+ verifyFormat("namespace ns {\n"
+ "class { /* comment */\n"
+ "} a, b;\n"
+ "} // namespace ns");
+ verifyFormat("namespace ns {\n"
+ "const class { /* comment */\n"
+ "} a, b;\n"
+ "} // namespace ns");
}
TEST_F(FormatTest, FormatsEnum) {
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 6d19af52dcd3e..0abe533dd5fc3 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -78,25 +78,38 @@ TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
TEST_F(TokenAnnotatorTest, UnderstandsClasses) {
auto Tokens = annotate("class C {};");
EXPECT_EQ(Tokens.size(), 6u) << Tokens;
- EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_ClassLBrace);
+
+ Tokens = annotate("const class C {} c;");
+ EXPECT_EQ(Tokens.size(), 8u) << Tokens;
+ EXPECT_TOKEN(Tokens[3], tok::l_brace, TT_ClassLBrace);
+
+ Tokens = annotate("const class {} c;");
+ EXPECT_EQ(Tokens.size(), 7u) << Tokens;
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_ClassLBrace);
}
TEST_F(TokenAnnotatorTest, UnderstandsStructs) {
auto Tokens = annotate("struct S {};");
EXPECT_EQ(Tokens.size(), 6u) << Tokens;
- EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_StructLBrace);
}
TEST_F(TokenAnnotatorTest, UnderstandsUnions) {
auto Tokens = annotate("union U {};");
EXPECT_EQ(Tokens.size(), 6u) << Tokens;
- EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_UnionLBrace);
+
+ Tokens = annotate("union U { void f() { return; } };");
+ EXPECT_EQ(Tokens.size(), 14u) << Tokens;
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_UnionLBrace);
+ EXPECT_TOKEN(Tokens[7], tok::l_brace, TT_FunctionLBrace);
}
TEST_F(TokenAnnotatorTest, UnderstandsEnums) {
auto Tokens = annotate("enum E {};");
EXPECT_EQ(Tokens.size(), 6u) << Tokens;
- EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+ EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_EnumLBrace);
}
TEST_F(TokenAnnotatorTest, UnderstandsLBracesInMacroDefinition) {
More information about the cfe-commits
mailing list