[PATCH] D43320: Allow dllimport non-type template arguments in C++17

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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