rsmith added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:670-672
+ /// Evaluate an expression that is required to be a core constant expression.
+ bool EvaluateAsCoreConstExpr(EvalResult &Result, QualType ParamType,
+ CCEKind CCE, const ASTContext &Ctx) const;
----------------
rnk wrote:
> rsmith wrote:
> > Seems strange to pass a converted constant expression kind to an 'evaluate
> > as core constant expression' function. And it seems like we don't need the
> > kind here, just an "evaluating for emission w/relocations" vs "evaluating
> > for mangling" enum.
> >
> > Also, we need to evaluate non-type template arguments as constant
> > expressions, not just as core constant expressions, which the
> > implementation does, but the name and comment here don't reflect. (The
> > difference is that you can't result in a pointer/reference to a temporary
> > or automatic / thread storage duration entity.)
> So... what are these things? Converted constant expressions? What are we
> evaluating them as? I guess they're not rvalues or lvalues.
I think this should just be called `EvaluateAsConstantExpr`, and you should
drop all the "core"s throughout. The difference between a constant expression
and a core constant expression is that a core constant expression allows the
evaluated value to refer to entities with automatic storage duration etc, which
is exactly the case you want to disallow here :)
I also don't think you need the `ParamType`, and can instead just look at
whether the expression itself is an rvalue.
================
Comment at: clang/include/clang/AST/Expr.h:663
+ bool EvaluateAsCoreConstExpr(EvalResult &Result, QualType ParamType,
+ bool IsTemplateArg, const ASTContext &Ctx)
const;
+
----------------
I would prefer an `enum { EvaluateForCodeGen, EvaluateForMangling }` over a
bool `IsTemplateArg`; I would not be surprised if we find other cases where we
want to evaluate an expression for mangling purposes only.
================
Comment at: clang/lib/AST/ExprConstant.cpp:707
+ /// Evaluate as a C++17 non-type template argument, which is a core
+ /// constant expression with a special case for dllimport declarations.
----------------
No "core" here.
================
Comment at: clang/lib/AST/ExprConstant.cpp:709
+ /// constant expression with a special case for dllimport declarations.
+ EM_TemplateArgument,
+
----------------
I don't think we need this. See below.
================
Comment at: clang/lib/AST/ExprConstant.cpp:10384-10385
+ if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
+ !CheckLValueConstantExpression(
+ Info, getExprLoc(), Ctx.getLValueReferenceType(getType()), LV))
+ return false;
----------------
Instead of a new `EvaluationMode` which is actually not used by evaluation, we
could pass a parameter into `CheckLValueConstantExpression` to indicate if
we're in the "for-mangling" mode.
================
Comment at: clang/lib/AST/ExprConstant.cpp:10397
+ EvalInfo Info(Ctx, Result, EM);
+ return ::EvaluateAsRValue(Info, this, Result.Val);
+}
----------------
If you switch from using `ParamType->isReferenceType()` to asking the
expression for its value category, you don't need the
implicit-lvalue-to-rvalue-conversion semantics of `EvaluateAsRValue`, and can
just call `::Evaluate` here followed by a call to `CheckConstantExpression`
(passing the latter the `IsTemplateArg` flag).
In fact, you don't need the separate case for lvalues above, either, since
`::Evaluate` does the right thing for an lvalue expression by itself.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5401
- if ((T->isReferenceType()
- ? !Result.get()->EvaluateAsLValue(Eval, S.Context)
- : !Result.get()->EvaluateAsRValue(Eval, S.Context)) ||
+ if (!Result.get()->EvaluateAsNonTypeTemplateArgument(Eval, T, S.Context) ||
(RequireInt && !Eval.Val.isInt())) {
----------------
rnk wrote:
> rsmith wrote:
> > Don't we get here for `CCEKind`s other than the non-type template argument
> > case?
> You're right, but I wasn't able to construct a test case where we would call
> `CheckConvertedConstantExpression` and we would reject it today because it is
> dllimported. This was my best idea, using `if constexpr`:
> ```
> struct __declspec(dllimport) Foo {
> static constexpr bool imported_foo = true;
> };
> const bool some_bool = false;
> const bool *f() {
> if constexpr (Foo::imported_foo) {
> return &Foo::imported_foo;
> } else {
> return &some_bool;
> }
> }
> ```
>
> Any other ideas on how to get more coverage of this path through
> CheckConvertedConstantExpression?
All the other forms of `CCEKind` also set `RequireInt` to `true`, and so any
case your check catches would also be caught on the line below.
https://reviews.llvm.org/D43320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
