[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv closed this revision. faisalv added a comment. Committed here : https://reviews.llvm.org/rL303492 https://reviews.llvm.org/D31588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv accepted this revision. faisalv added a comment. This revision is now accepted and ready to land. Modified and committed as: http://llvm.org/viewvc/llvm-project?view=revision=303492 https://reviews.llvm.org/D31588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
rsmith added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:203-222
+ // Create the ExpressionEvaluationContext on the stack - but only if asked
to.
+ struct EnterExpressionEvaluationContextConditionalRAII {
+llvm::AlignedCharArray
+MyStackStorage;
+const EnterExpressionEvaluationContext *const ConstantEvaluatedContext;
+EnterExpressionEvaluationContextConditionalRAII(bool CreateIt,
This seems more complexity than we need. How about factoring out a
`ParseConstantExpressionInExprEvalContext` function that doesn't create a
context, and then calling it from this function after creating the context?
Comment at: lib/Parse/ParseTemplate.cpp:1208-1233
if (isCXXTypeId(TypeIdAsTemplateArgument)) {
SourceLocation Loc = Tok.getLocation();
-TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
- Declarator::TemplateTypeArgContext);
+TypeResult TypeArg =
+ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
if (TypeArg.isInvalid())
return ParsedTemplateArgument();
+
There's a bunch of whitespace changes here. I have no objection to them but
they should be handled separately rather than mixed into this change.
https://reviews.llvm.org/D31588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv added a comment. *ping* *ping* https://reviews.llvm.org/D31588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv added a comment. *ping* https://reviews.llvm.org/D31588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv updated this revision to Diff 96861.
faisalv added a comment.
Fixed a regression test that should have passed without emitting error
diagnostics - and now does.
https://reviews.llvm.org/D31588
Files:
include/clang/Parse/Parser.h
lib/Parse/ParseExpr.cpp
lib/Parse/ParseTemplate.cpp
test/SemaCXX/lambda-expressions.cpp
test/SemaCXX/local-classes.cpp
Index: test/SemaCXX/local-classes.cpp
===
--- test/SemaCXX/local-classes.cpp
+++ test/SemaCXX/local-classes.cpp
@@ -40,3 +40,15 @@
};
}
}
+
+namespace PR25627_dont_odr_use_local_consts {
+ template struct X {};
+
+ void foo() {
+const int N = 10;
+
+struct Local {
+ void f(X) {} // OK
+};
+ }
+}
Index: test/SemaCXX/lambda-expressions.cpp
===
--- test/SemaCXX/lambda-expressions.cpp
+++ test/SemaCXX/lambda-expressions.cpp
@@ -525,9 +525,9 @@
class S {};
void foo() {
- const int num = 18; // expected-note {{'num' declared here}}
+ const int num = 18;
auto outer = []() {
-auto inner = [](S ) {}; // expected-error {{variable 'num' cannot be implicitly captured in a lambda with no capture-default specified}}
+auto inner = [](S ) {};
};
}
}
@@ -573,3 +573,13 @@
auto s1 = S1{[name=name]() {}}; // expected-error {{use of undeclared identifier 'name'; did you mean 'name1'?}}
}
}
+
+namespace PR25627_dont_odr_use_local_consts {
+
+ template struct X {};
+
+ void foo() {
+const int N = 10;
+(void) [] { X x; };
+ }
+}
Index: lib/Parse/ParseTemplate.cpp
===
--- lib/Parse/ParseTemplate.cpp
+++ lib/Parse/ParseTemplate.cpp
@@ -1198,42 +1198,49 @@
// expression is resolved to a type-id, regardless of the form of
// the corresponding template-parameter.
//
- // Therefore, we initially try to parse a type-id.
+ // Therefore, we initially try to parse a type-id - and isCXXTypeId might look
+ // up and annotate an identifier as an id-expression during disambiguation,
+ // so enter the appropriate context for a constant expression template
+ // argument before trying to disambiguate.
+
+ EnterExpressionEvaluationContext EnterConstantEvaluated(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
if (isCXXTypeId(TypeIdAsTemplateArgument)) {
SourceLocation Loc = Tok.getLocation();
-TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
- Declarator::TemplateTypeArgContext);
+TypeResult TypeArg =
+ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
if (TypeArg.isInvalid())
return ParsedTemplateArgument();
-
+
return ParsedTemplateArgument(ParsedTemplateArgument::Type,
- TypeArg.get().getAsOpaquePtr(),
- Loc);
+ TypeArg.get().getAsOpaquePtr(), Loc);
}
-
// Try to parse a template template argument.
{
TentativeParsingAction TPA(*this);
-ParsedTemplateArgument TemplateTemplateArgument
- = ParseTemplateTemplateArgument();
+ParsedTemplateArgument TemplateTemplateArgument =
+ParseTemplateTemplateArgument();
if (!TemplateTemplateArgument.isInvalid()) {
TPA.Commit();
return TemplateTemplateArgument;
}
-
+
// Revert this tentative parse to parse a non-type template argument.
TPA.Revert();
}
-
- // Parse a non-type template argument.
+
+ // Parse a non-type template argument.
SourceLocation Loc = Tok.getLocation();
- ExprResult ExprArg = ParseConstantExpression(MaybeTypeCast);
+ ExprResult ExprArg = ParseConstantExpression(
+ MaybeTypeCast,
+ false /*don't create another ConstantExpressionEvaluationContext,
+use ours instead*/);
if (ExprArg.isInvalid() || !ExprArg.get())
return ParsedTemplateArgument();
- return ParsedTemplateArgument(ParsedTemplateArgument::NonType,
-ExprArg.get(), Loc);
+ return ParsedTemplateArgument(ParsedTemplateArgument::NonType, ExprArg.get(),
+Loc);
}
/// \brief Determine whether the current tokens can only be parsed as a
@@ -1274,16 +1281,16 @@
/// template-argument-list ',' template-argument
bool
Parser::ParseTemplateArgumentList(TemplateArgList ) {
- // Template argument lists are constant-evaluation contexts.
- EnterExpressionEvaluationContext EvalContext(
- Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
ColonProtectionRAIIObject ColonProtection(*this, false);
do {
ParsedTemplateArgument Arg = ParseTemplateArgument();
SourceLocation EllipsisLoc;
-if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
+ // FIXME: Do we need need to enter a
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv updated this revision to Diff 96858.
faisalv marked 3 inline comments as done.
faisalv added a comment.
Updated the patch following Richard's feedback:
- teach ParseConstantExpression to create its own ExpressionEvaluationContext
only if asked to.
https://reviews.llvm.org/D31588
Files:
include/clang/Parse/Parser.h
lib/Parse/ParseExpr.cpp
lib/Parse/ParseTemplate.cpp
test/SemaCXX/lambda-expressions.cpp
test/SemaCXX/local-classes.cpp
Index: test/SemaCXX/local-classes.cpp
===
--- test/SemaCXX/local-classes.cpp
+++ test/SemaCXX/local-classes.cpp
@@ -40,3 +40,15 @@
};
}
}
+
+namespace PR25627_dont_odr_use_local_consts {
+ template struct X {};
+
+ void foo() {
+const int N = 10;
+
+struct Local {
+ void f(X) {} // OK
+};
+ }
+}
Index: test/SemaCXX/lambda-expressions.cpp
===
--- test/SemaCXX/lambda-expressions.cpp
+++ test/SemaCXX/lambda-expressions.cpp
@@ -573,3 +573,13 @@
auto s1 = S1{[name=name]() {}}; // expected-error {{use of undeclared identifier 'name'; did you mean 'name1'?}}
}
}
+
+namespace PR25627_dont_odr_use_local_consts {
+
+ template struct X {};
+
+ void foo() {
+const int N = 10;
+(void) [] { X x; };
+ }
+}
Index: lib/Parse/ParseTemplate.cpp
===
--- lib/Parse/ParseTemplate.cpp
+++ lib/Parse/ParseTemplate.cpp
@@ -1198,42 +1198,48 @@
// expression is resolved to a type-id, regardless of the form of
// the corresponding template-parameter.
//
- // Therefore, we initially try to parse a type-id.
+ // Therefore, we initially try to parse a type-id - and isCXXTypeId might look
+ // up and annotate an identifier as an id - expression during disambiguation,
+ // so enter the appropriate context for a constant expression template
+ // argument before trying to disambiguate.
+
+ EnterExpressionEvaluationContext EnterConstantEvaluated(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
if (isCXXTypeId(TypeIdAsTemplateArgument)) {
SourceLocation Loc = Tok.getLocation();
-TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
- Declarator::TemplateTypeArgContext);
+TypeResult TypeArg =
+ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
if (TypeArg.isInvalid())
return ParsedTemplateArgument();
-
+
return ParsedTemplateArgument(ParsedTemplateArgument::Type,
- TypeArg.get().getAsOpaquePtr(),
- Loc);
+ TypeArg.get().getAsOpaquePtr(), Loc);
}
-
// Try to parse a template template argument.
{
TentativeParsingAction TPA(*this);
-ParsedTemplateArgument TemplateTemplateArgument
- = ParseTemplateTemplateArgument();
+ParsedTemplateArgument TemplateTemplateArgument =
+ParseTemplateTemplateArgument();
if (!TemplateTemplateArgument.isInvalid()) {
TPA.Commit();
return TemplateTemplateArgument;
}
-
+
// Revert this tentative parse to parse a non-type template argument.
TPA.Revert();
}
-
- // Parse a non-type template argument.
+
+ // Parse a non-type template argument.
SourceLocation Loc = Tok.getLocation();
- ExprResult ExprArg = ParseConstantExpression(MaybeTypeCast);
+ ExprResult ExprArg = ParseConstantExpression(
+ MaybeTypeCast,
+ true /*Don't create another ConstantExpressionEvaluationContext*/);
if (ExprArg.isInvalid() || !ExprArg.get())
return ParsedTemplateArgument();
- return ParsedTemplateArgument(ParsedTemplateArgument::NonType,
-ExprArg.get(), Loc);
+ return ParsedTemplateArgument(ParsedTemplateArgument::NonType, ExprArg.get(),
+Loc);
}
/// \brief Determine whether the current tokens can only be parsed as a
@@ -1274,16 +1280,16 @@
/// template-argument-list ',' template-argument
bool
Parser::ParseTemplateArgumentList(TemplateArgList ) {
- // Template argument lists are constant-evaluation contexts.
- EnterExpressionEvaluationContext EvalContext(
- Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
ColonProtectionRAIIObject ColonProtection(*this, false);
do {
ParsedTemplateArgument Arg = ParseTemplateArgument();
SourceLocation EllipsisLoc;
-if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
+ // FIXME: Do we need need to enter a constant evaluation context here?
Arg = Actions.ActOnPackExpansion(Arg, EllipsisLoc);
+}
if (Arg.isInvalid()) {
SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);
Index: lib/Parse/ParseExpr.cpp
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
rsmith added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:1203-1204
+ {
+EnterExpressionEvaluationContext EnterConstantEvaluated(
+Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+if (isCXXTypeId(TypeIdAsTemplateArgument)) {
Please add a comment here, something like:
isCXXTypeId might look up and annotate an identifier as an id-expression during
disambiguation, so enter the appropriate context for a constant expression
template argument before trying to disambiguate.
Comment at: lib/Parse/ParseTemplate.cpp:1214-1243
+} else {
+ // If we disambiguated as an expression that we identified as potentially
+ // not being odr-used (consistent with a template argument context), and
+ // annotated our token as that expression, then remove it from the
+ // MaybeODRUsedExprs so that it doesn't trigger a false error, since it
+ // would otherwise have been removed when completing processing of a
+ // constant expression.
... we shouldn't need to do any of this: instead, keep your
`ExprEvaluationContext` alive through the call to `ParseConstantExpression`,
and tell it to not create its own context in this case.
Comment at: lib/Parse/ParseTemplate.cpp:1234-1237
+// it for later since we can be certain that this expression would
+// eventually have been removed during ActOnConstantExpression called
+// from ParseConstantExpression when parsing the non-type template
+// argument below. Eitherway, the appropriate checking for an
This doesn't seem right. If the template parameter is of reference type, the
named entity should be considered to be odr-used. And likewise just because we
stopped disambiguation after finding an id-expression that names a non-type,
that does not imply that the overall template argument is the entity named by
that id-expression. (Eg, consider `X`, where `F` is a functor whose
`operator()` returns `this` -- that template argument should be considered to
odr-use `F`.) But...
Repository:
rL LLVM
https://reviews.llvm.org/D31588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)
faisalv created this revision.
This patch ensures that clang processes the expression-nodes that are generated
when disambiguating between types and expressions within template arguments, as
if they were truly constant-expressions. Currently, trunk correctly
disambiguates, and identifies the expression as an expression - and while it
annotates the token with the expression - it fails to complete the odr-use
processing (specifically, failing to trigger
Sema::UpdateMarkingForLValueToRValue as is done so for all Constant
Expressions, which removes it from being considered odr-used).
For e.g:
template struct X { };
void f() {
const int N = 10;
X x; // should be OK.
[] { return X{}; }; // also OK - no capture.
}
See a related bug: https://bugs.llvm.org//show_bug.cgi?id=25627
The fix is as follows:
- Remove the EnteredConstantEvaluatedContext action from
ParseTemplateArgumentList (relying that ParseTemplateArgument will get it right)
- Add the EnteredConstantEvaluatedContext action just prior to undergoing the
disambiguating parse, and if the parse succeeds for an expression, make sure it
doesn't linger within MaybeODRUsedExprs by clearing it (while asserting that it
only contains the offending expression)
I need to add some tests... and fix one regression.
Does the approach look sound?
Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D31588
Files:
lib/Parse/ParseTemplate.cpp
Index: lib/Parse/ParseTemplate.cpp
===
--- lib/Parse/ParseTemplate.cpp
+++ lib/Parse/ParseTemplate.cpp
@@ -1198,42 +1198,73 @@
// expression is resolved to a type-id, regardless of the form of
// the corresponding template-parameter.
//
- // Therefore, we initially try to parse a type-id.
- if (isCXXTypeId(TypeIdAsTemplateArgument)) {
-SourceLocation Loc = Tok.getLocation();
-TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
- Declarator::TemplateTypeArgContext);
-if (TypeArg.isInvalid())
- return ParsedTemplateArgument();
-
-return ParsedTemplateArgument(ParsedTemplateArgument::Type,
- TypeArg.get().getAsOpaquePtr(),
- Loc);
- }
-
+ // Therefore, we initially try to parse a type-id.
+ {
+EnterExpressionEvaluationContext EnterConstantEvaluated(
+Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+if (isCXXTypeId(TypeIdAsTemplateArgument)) {
+ SourceLocation Loc = Tok.getLocation();
+ TypeResult TypeArg =
+ ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
+ if (TypeArg.isInvalid())
+return ParsedTemplateArgument();
+
+ return ParsedTemplateArgument(ParsedTemplateArgument::Type,
+TypeArg.get().getAsOpaquePtr(), Loc);
+} else {
+ // If we disambiguated as an expression that we identified as potentially
+ // not being odr-used (consistent with a template argument context), and
+ // annotated our token as that expression, then remove it from the
+ // MaybeODRUsedExprs so that it doesn't trigger a false error, since it
+ // would otherwise have been removed when completing processing of a
+ // constant expression.
+ if (Actions.MaybeODRUseExprs.size()) {
+assert(Actions.MaybeODRUseExprs.size() == 1 &&
+ "we should only need to parse one expression to determine an "
+ "error or typeid");
+assert(Tok.getKind() == tok::annot_primary_expr &&
+ getExprAnnotation(Tok).get() ==
+ *Actions.MaybeODRUseExprs.begin() &&
+ "The expression (DeclRefExpr or MemExpr) stored within "
+ "MaybeODRUseExprs for later checking whether we perform an "
+ "lvalue-to-rvalue conversion before determining odr-use must be "
+ "the same as the annotated token during ambiguity resolution");
+// We clear this state, since this lingering partially processed
+// expression should not trigger an error now and we don't need to save
+// it for later since we can be certain that this expression would
+// eventually have been removed during ActOnConstantExpression called
+// from ParseConstantExpression when parsing the non-type template
+// argument below. Eitherway, the appropriate checking for an
+// appropriate constant-expression that matches the
+// non-type-template-parameter occurs later in CheckTemplateArgument.
+Actions.MaybeODRUseExprs.clear();
+ }
+}
+ } // End ExpressionEvaluationContext
+
// Try to parse a template template argument.
{
TentativeParsingAction TPA(*this);
-ParsedTemplateArgument TemplateTemplateArgument
- = ParseTemplateTemplateArgument();
+ParsedTemplateArgument TemplateTemplateArgument =
+
