[clang] b1eeec6 - [clang-format] Remove special logic for parsing concept definitions.
Emilia Dreamer via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 5 19:18:34 PST 2023
Author: Emilia Dreamer
Date: 2023-01-06T05:17:58+02:00
New Revision: b1eeec6177fafcc433d98c4f46f353b13c68aca0
URL: https://github.com/llvm/llvm-project/commit/b1eeec6177fafcc433d98c4f46f353b13c68aca0
DIFF: https://github.com/llvm/llvm-project/commit/b1eeec6177fafcc433d98c4f46f353b13c68aca0.diff
LOG: [clang-format] Remove special logic for parsing concept definitions.
Previously, clang-format relied on a special method to parse concept
definitions, `UnwrappedLineParser::parseConcept()`, which deferred to
`UnwrappedLineParser::parseConstraintExpression()`. This is problematic,
because the C++ grammar treats concepts and requires clauses
differently, causing issues such as https://github.com/llvm/llvm-project/issues/55898 and https://github.com/llvm/llvm-project/issues/58130.
This patch removes `parseConcept`, letting the formatter parse concept
definitions as more like what they actually are, fancy bool definitions.
NOTE that because of this, some long concept definitions change in their
formatting, as can be seen in the changed tests. This is because of a
change in split penalties, caused by a change in MightBeFunctionDecl on
the concept definition line, which was previously `true` but with this
patch is now `false`.
One might argue that `false` is a more "correct" value for concept
definitions, but I'd be fine with setting it to `true` again to maintain
compatibility with previous versions.
Fixes https://github.com/llvm/llvm-project/issues/58130
Depends on D140330
Reviewed By: HazardyKnusperkeks, owenpan, MyDeveloperDay
Differential Revision: https://reviews.llvm.org/D140339
Added:
Modified:
clang/lib/Format/TokenAnnotator.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 35fa8cf91b33..9dd92c9b08f1 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1690,8 +1690,8 @@ class AnnotatingParser {
if (!Tok)
return false;
- if (Tok->isOneOf(tok::kw_class, tok::kw_enum, tok::kw_concept,
- tok::kw_struct, tok::kw_using)) {
+ if (Tok->isOneOf(tok::kw_class, tok::kw_enum, tok::kw_struct,
+ tok::kw_using)) {
return false;
}
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 8e1ea0677902..81a6d8ffc0ee 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -1818,9 +1818,6 @@ void UnwrappedLineParser::parseStructuralElement(
break;
}
break;
- case tok::kw_concept:
- parseConcept();
- return;
case tok::kw_requires: {
if (Style.isCpp()) {
bool ParsedClause = parseRequires();
@@ -3277,26 +3274,6 @@ void UnwrappedLineParser::parseAccessSpecifier() {
}
}
-/// \brief Parses a concept definition.
-/// \pre The current token has to be the concept keyword.
-///
-/// Returns if either the concept has been completely parsed, or if it detects
-/// that the concept definition is incorrect.
-void UnwrappedLineParser::parseConcept() {
- assert(FormatTok->is(tok::kw_concept) && "'concept' expected");
- nextToken();
- if (!FormatTok->is(tok::identifier))
- return;
- nextToken();
- if (!FormatTok->is(tok::equal))
- return;
- nextToken();
- parseConstraintExpression();
- if (FormatTok->is(tok::semi))
- nextToken();
- addUnwrappedLine();
-}
-
/// \brief Parses a requires, decides if it is a clause or an expression.
/// \pre The current token has to be the requires keyword.
/// \returns true if it parsed a clause.
@@ -3463,6 +3440,8 @@ void UnwrappedLineParser::parseRequiresClause(FormatToken *RequiresToken) {
? TT_RequiresClauseInARequiresExpression
: TT_RequiresClause);
+ // NOTE: parseConstraintExpression is only ever called from this function.
+ // It could be inlined into here.
parseConstraintExpression();
if (!InRequiresExpression)
@@ -3496,9 +3475,8 @@ void UnwrappedLineParser::parseRequiresExpression(FormatToken *RequiresToken) {
/// \brief Parses a constraint expression.
///
-/// This is either the definition of a concept, or the body of a requires
-/// clause. It returns, when the parsing is complete, or the expression is
-/// incorrect.
+/// This is the body of a requires clause. It returns, when the parsing is
+/// complete, or the expression is incorrect.
void UnwrappedLineParser::parseConstraintExpression() {
// The special handling for lambdas is needed since tryToParseLambda() eats a
// token and if a requires expression is the last part of a requires clause
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index ce59180e9032..97c5baf152fe 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -158,7 +158,6 @@ class UnwrappedLineParser {
void parseAccessSpecifier();
bool parseEnum();
bool parseStructLike();
- void parseConcept();
bool parseRequires();
void parseRequiresClause(FormatToken *RequiresToken);
void parseRequiresExpression(FormatToken *RequiresToken);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 843e945aeae5..21b497bd9568 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -23588,10 +23588,10 @@ TEST_F(FormatTest, Concepts) {
"concept DelayedCheck = !!false || requires(T t) { t.bar(); } "
"&& sizeof(T) <= 8;");
- verifyFormat(
- "template <typename T>\n"
- "concept DelayedCheck = static_cast<bool>(0) ||\n"
- " requires(T t) { t.bar(); } && sizeof(T) <= 8;");
+ verifyFormat("template <typename T>\n"
+ "concept DelayedCheck =\n"
+ " static_cast<bool>(0) || requires(T t) { t.bar(); } && "
+ "sizeof(T) <= 8;");
verifyFormat("template <typename T>\n"
"concept DelayedCheck = bool(0) || requires(T t) { t.bar(); } "
@@ -23599,8 +23599,8 @@ TEST_F(FormatTest, Concepts) {
verifyFormat(
"template <typename T>\n"
- "concept DelayedCheck = (bool)(0) ||\n"
- " requires(T t) { t.bar(); } && sizeof(T) <= 8;");
+ "concept DelayedCheck =\n"
+ " (bool)(0) || requires(T t) { t.bar(); } && sizeof(T) <= 8;");
verifyFormat("template <typename T>\n"
"concept DelayedCheck = (bool)0 || requires(T t) { t.bar(); } "
@@ -23636,6 +23636,9 @@ TEST_F(FormatTest, Concepts) {
verifyFormat("template <typename T>\n"
"concept True = S<T>::Value;");
+ verifyFormat("template <S T>\n"
+ "concept True = T.field;");
+
verifyFormat(
"template <typename T>\n"
"concept C = []() { return true; }() && requires(T t) { t.bar(); } &&\n"
@@ -23914,11 +23917,9 @@ TEST_F(FormatTest, Concepts) {
verifyFormat("template <typename T> concept True = true;", Style);
verifyFormat(
- "template <typename T> concept C = decltype([]() -> std::true_type {\n"
- " return {};\n"
- " }())::value &&\n"
- " requires(T t) { t.bar(); } && "
- "sizeof(T) <= 8;",
+ "template <typename T> concept C =\n"
+ " decltype([]() -> std::true_type { return {}; }())::value &&\n"
+ " requires(T t) { t.bar(); } && sizeof(T) <= 8;",
Style);
verifyFormat("template <typename T> concept Semiregular =\n"
@@ -23936,21 +23937,20 @@ TEST_F(FormatTest, Concepts) {
// The following tests are invalid C++, we just want to make sure we don't
// assert.
- verifyFormat("template <typename T>\n"
- "concept C = requires C2<T>;");
+ verifyNoCrash("template <typename T>\n"
+ "concept C = requires C2<T>;");
- verifyFormat("template <typename T>\n"
- "concept C = 5 + 4;");
+ verifyNoCrash("template <typename T>\n"
+ "concept C = 5 + 4;");
- verifyFormat("template <typename T>\n"
- "concept C =\n"
- "class X;");
+ verifyNoCrash("template <typename T>\n"
+ "concept C = class X;");
- verifyFormat("template <typename T>\n"
- "concept C = [] && true;");
+ verifyNoCrash("template <typename T>\n"
+ "concept C = [] && true;");
- verifyFormat("template <typename T>\n"
- "concept C = [] && requires(T t) { typename T::size_type; };");
+ verifyNoCrash("template <typename T>\n"
+ "concept C = [] && requires(T t) { typename T::size_type; };");
}
TEST_F(FormatTest, RequiresClausesPositions) {
More information about the cfe-commits
mailing list