-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Clang][NFC] Consolidate the parameter check for the requires
expression parameter.
#110773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
requires
expression parameter.
@llvm/pr-subscribers-clang Author: None (c8ef) ChangesThis patch is a follow-up to #109831. In the discussion, we agreed that having parameter checks scattered across different areas isn't ideal. Therefore, I suggest merging the check from #88974 into the void parameter check. This change won't impact functionality and will enhance maintainability. Full diff: https://github.com/llvm/llvm-project/pull/110773.diff 2 Files Affected:
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index a04eed9873c0d4..122a05be1c039a 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -7950,21 +7950,8 @@ void Parser::ParseParameterDeclarationClause(
// Parse a C++23 Explicit Object Parameter
// We do that in all language modes to produce a better diagnostic.
SourceLocation ThisLoc;
- if (getLangOpts().CPlusPlus && Tok.is(tok::kw_this)) {
+ if (getLangOpts().CPlusPlus && Tok.is(tok::kw_this))
ThisLoc = ConsumeToken();
- // C++23 [dcl.fct]p6:
- // An explicit-object-parameter-declaration is a parameter-declaration
- // with a this specifier. An explicit-object-parameter-declaration
- // shall appear only as the first parameter-declaration of a
- // parameter-declaration-list of either:
- // - a member-declarator that declares a member function, or
- // - a lambda-declarator.
- //
- // The parameter-declaration-list of a requires-expression is not such
- // a context.
- if (DeclaratorCtx == DeclaratorContext::RequiresExpr)
- Diag(ThisLoc, diag::err_requires_expr_explicit_object_parameter);
- }
ParsedTemplateInfo TemplateInfo;
ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index b30414a8a8277a..d490452e91c3bb 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -9519,15 +9519,28 @@ Sema::ActOnStartRequiresExpr(SourceLocation RequiresKWLoc,
} else if (Param->getType().hasQualifiers()) {
Diag(Param->getBeginLoc(), diag::err_void_param_qualified);
}
- }
-
- if (Param->hasDefaultArg())
+ } else if (Param->hasDefaultArg()) {
// C++2a [expr.prim.req] p4
// [...] A local parameter of a requires-expression shall not have a
// default argument. [...]
Diag(Param->getDefaultArgRange().getBegin(),
diag::err_requires_expr_local_parameter_default_argument);
- // Ignore default argument and move on
+ // Ignore default argument and move on
+ } else if (Param->isExplicitObjectParameter()) {
+ // C++23 [dcl.fct]p6:
+ // An explicit-object-parameter-declaration is a parameter-declaration
+ // with a this specifier. An explicit-object-parameter-declaration
+ // shall appear only as the first parameter-declaration of a
+ // parameter-declaration-list of either:
+ // - a member-declarator that declares a member function, or
+ // - a lambda-declarator.
+ //
+ // The parameter-declaration-list of a requires-expression is not such
+ // a context.
+ Diag(Param->getExplicitObjectParamThisLoc(),
+ diag::err_requires_expr_explicit_object_parameter);
+ Param->setExplicitObjectParameterLoc(SourceLocation());
+ }
Param->setDeclContext(Body);
// If this has an identifier, add it to the scope stack.
|
Thank you so much for your quick review! However, it seems the current CI is blocked by the issues mentioned in #110783 :( |
I wouldn't be worried about that for a simple change like this, you can go ahead and merge. |
Thanks! 🥰 |
// a context. | ||
Diag(Param->getExplicitObjectParamThisLoc(), | ||
diag::err_requires_expr_explicit_object_parameter); | ||
Param->setExplicitObjectParameterLoc(SourceLocation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why clear the source location? Wouldn't it somehow break the source fidelity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm-project/clang/lib/Sema/SemaType.cpp
Lines 5175 to 5179 in e379094
if (Param->isExplicitObjectParameter()) { | |
S.Diag(Param->getLocation(), | |
diag::err_void_explicit_object_param); | |
Param->setExplicitObjectParameterLoc(SourceLocation()); | |
} |
I'm not entirely sure, but the check in function parameters do indeed perform this action.
…ssion parameter. (llvm#110773) This patch is a follow-up to llvm#109831. In the discussion, we agreed that having parameter checks scattered across different areas isn't ideal. Therefore, I suggest merging the check from llvm#88974 into the void parameter check. This change won't impact functionality and will enhance maintainability.
This patch is a follow-up to #109831. In the discussion, we agreed that having parameter checks scattered across different areas isn't ideal. Therefore, I suggest merging the check from #88974 into the void parameter check. This change won't impact functionality and will enhance maintainability.