[clang-format] Make bitwise and imply requires clause #110942
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch adjusts the requires clause/expression lookahead heuristic to assume that some binary operation mean that the requires keyword refers to a clause. This partially reverts the removed case clauses from 9b68c09, with an additional check that we're not in a type parameter. This is quite a band-aid fix, it may be worth it to rewrite that whole heuristic to track more state in the future, instead of just blindly marching forward across multiple unrelated definitions, since right now, the definition following the one with the requires clause can influence whether the heuristic chooses clause or expression. Fixes llvm#110485
|
@llvm/pr-subscribers-clang-format Author: Emilia Kond (rymiel) ChangesThis patch adjusts the requires clause/expression lookahead heuristic to assume that some binary operation mean that the requires keyword refers to a clause. This partially reverts the removed case clauses from 9b68c09, with an additional check that we're not in a type parameter. This is quite a band-aid fix, it may be worth it to rewrite that whole heuristic to track more state in the future, instead of just blindly marching forward across multiple unrelated definitions, since right now, the definition following the one with the requires clause can influence whether the heuristic chooses clause or expression. Fixes #110485 Full diff: https://github.com/llvm/llvm-project/pull/110942.diff 2 Files Affected:
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 40f77266fabdca..98078fb50dcfaa 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -3528,6 +3528,17 @@ bool UnwrappedLineParser::parseRequires() {
return false;
}
break;
+ case tok::equalequal:
+ case tok::greaterequal:
+ case tok::lessequal:
+ case tok::r_paren:
+ case tok::pipepipe:
+ if (OpenAngles == 0) {
+ FormatTok = Tokens->setPosition(StoredPosition);
+ parseRequiresClause(RequiresToken);
+ return true;
+ }
+ break;
case tok::eof:
// Break out of the loop.
Lookahead = 50;
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 1884d41a5f23f5..6f1b1df0bda655 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1296,6 +1296,21 @@ TEST_F(TokenAnnotatorTest, UnderstandsRequiresClausesAndConcepts) {
Tokens = annotate("bool x = t && requires(Foo<C1 || C2> x) { x.foo(); };");
ASSERT_EQ(Tokens.size(), 25u) << Tokens;
EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresExpression);
+
+ // Second function definition is required due to lookahead
+ Tokens = annotate("void f() & requires(n == 1) {}\nvoid g();");
+ ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+ EXPECT_TOKEN(Tokens[4], tok::amp, TT_PointerOrReference);
+ EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresClause);
+
+ Tokens = annotate("void f() & requires(n || h) {}\nvoid g();");
+ ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+ EXPECT_TOKEN(Tokens[4], tok::amp, TT_PointerOrReference);
+ EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresClause);
+
+ Tokens = annotate("bool x = t && requires(F<n == 1> x) { x.foo(); };");
+ ASSERT_EQ(Tokens.size(), 25u) << Tokens;
+ EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresExpression);
}
TEST_F(TokenAnnotatorTest, UnderstandsRequiresExpressions) {
|
owenca
reviewed
Oct 3, 2024
Comment on lines
3531
to
3541
| case tok::equalequal: | ||
| case tok::greaterequal: | ||
| case tok::lessequal: | ||
| case tok::r_paren: | ||
| case tok::pipepipe: | ||
| if (OpenAngles == 0) { | ||
| FormatTok = Tokens->setPosition(StoredPosition); | ||
| parseRequiresClause(RequiresToken); | ||
| return true; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Instead, how about just moving case tok::amp: up?
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -3474,10 +3474,10 @@ bool UnwrappedLineParser::parseRequires() {
case tok::r_paren:
case tok::kw_noexcept:
case tok::kw_const:
+ case tok::amp:
// This is a requires clause.
parseRequiresClause(RequiresToken);
return true;
- case tok::amp:
case tok::ampamp: {
// This can be either:
// if (... && requires (T t) ...)
The rationale is that it doesn't make sense to bitwise-and another expression with a requires expression.
There was a problem hiding this comment.
Then wouldn't r-value reference methods still be broken? Those still exist even if they're rare. I mean, we can do both
There was a problem hiding this comment.
My suggested change (in place of yours) passes all the existing tests and the new tests you add here. Can you give a counterexample?
There was a problem hiding this comment.
What about the original issue but with an r-value ref-qualifier instead of an l-value one:
template <int n> struct S
{
void f() && requires(n == 1) {}
// some comment
void g() {}
};There was a problem hiding this comment.
Nvm. This would fail:
template <int n> struct S
{
void f() &&
requires(n == 1)
{
}
// some comment
void g() {}
};
There was a problem hiding this comment.
What about < and >?
template <int n> struct S {
void f() &
requires(n < 1)
{}
// some comment
void g() {}
};
There was a problem hiding this comment.
It gets complicated with template parameters :(
Like I said in the pr description, it's probably a better idea to redo that whole lookahead thing to be a little more smart. But I also don't have the energy for that so I just offered this bandaid fix
There was a problem hiding this comment.
If we are doing a limited fix now, maybe just move the case tok::amp: up as suggested above? It would cover patterns similar to that in #110485 no matter what operator is used in the parenthesized expression after & requires.
rymiel
changed the title
owenca
reviewed
Oct 11, 2024
owenca
approved these changes
Oct 22, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/7933 Here is the relevant piece of the build log for the reference |
| @@ -1296,6 +1296,15 @@ TEST_F(TokenAnnotatorTest, UnderstandsRequiresClausesAndConcepts) { | |||
| Tokens = annotate("bool x = t && requires(Foo<C1 || C2> x) { x.foo(); };"); | |||
| ASSERT_EQ(Tokens.size(), 25u) << Tokens; | |||
| EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresExpression); | |||
|
|
|||
| // Second function definition is required due to lookahead | |||
| Tokens = annotate("void f() &\n" | |||
There was a problem hiding this comment.
I'm not against rewriting the heuristic, if you know a better one. But for this example could one not extent the look behind? Than it would also work for r-value references.
Or try to annotate the reference marker and match on that?
There was a problem hiding this comment.
I'm not against rewriting the heuristic, if you know a better one. But for this example could one not extent the look behind? Than it would also work for r-value references.
No, because we can't tell if the r_paren before && requires is a function decl r_paren?
Or try to annotate the reference marker and match on that?
No, because we're in UnwrappedLineParser?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adjusts the requires clause/expression parser to imply a requires clause if it is preceded by a bitwise and operator
&, and assume it is a reference qualifier. The justification is that bitwise operations should not be used for requires expressions.This is a band-aid fix. The real problems lie in the lookahead heuristic in the same method. It may be worth it to rewrite that whole heuristic to track more state in the future, instead of just blindly marching forward across multiple unrelated definitions, since right now, the definition following the one with the requires clause can influence whether the heuristic chooses clause or expression.
Fixes #110485