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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10166-10182
+    EvalInfo Info(Ctx, Result, EvalInfo::EM_ConstantFold);
+    Info.IsNonTypeTemplateArgument = true;
+    LValue LV;
+    if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
+        !CheckLValueConstantExpression(
+            Info, getExprLoc(), Ctx.getLValueReferenceType(getType()), LV))
+      return false;
----------------
rsmith wrote:
> rsmith wrote:
> > Neither `EM_ConstantFold` nor `EM_IgnoreSideEffects` seem like the right evaluation modes to be using to evaluate a non-type template argument. I would expect `EM_ConstantExpression` to be used for both cases.
> Oh, I see, we're allowing side-effects here and then issuing a constant-folding warning elsewhere if there actually were any. *sigh*
> 
> I would expect we can get away with not doing that for template arguments. It'd be worth testing whether GCC actually allows constant folding there, or requires a real constant expression.
"Allowing side effects" here looks like it really means allowing non-constant subexpressions when those subexpressions would be discarded:

```
template <int *N> struct Foo {};
int x;
Foo<(x, &x)> f;

constexpr bool is_true = true;

int *fn() {
  if constexpr (((void)x, &x) != nullptr) {
    return &x;
  } else {
    return nullptr;
  }

  if constexpr (is_true || x == 2) {
    return &x;
  } else {
    return nullptr;
  }
}
```

Clang accepts this, GCC rejects a few. I'm guessing we want that kind of behavior for `if constexpr` conditions, but maybe not for non-type template parameters.

So, it sounds like we should check the CCE kind in SemaOverload, and use a more restrictive constant expression evaluation mode in that case. Perhaps even a new one for non-type template arguments.


================
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())) {
----------------
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?


https://reviews.llvm.org/D43320