[PATCH] D16465: [MS ABI] Prevent some expressions from evaluating to a constant

[Ehsan Akhgari via cfe-commits]

ehsan added a comment.

In http://reviews.llvm.org/D16465#333688, @rnk wrote:

> Your code won't catch this test case:
>
>   static int x;
>   extern inline const bool *f() {
>     static const bool p = !&x;
>     return &p;
>   }
>
> Getting this exactly right is going to be a challenge. =/


Oh you're right.  I need to spend some more time comparing our behavior with cl's, it seems...


================
Comment at: include/clang/Basic/DiagnosticASTKinds.td:151
@@ -150,1 +150,3 @@
   "%plural{1:byte|:bytes}1">;
+def note_constexpr_microsoft_abi_declrefexpr : Note<
+  "the constant expression cannot contain a reference to a variable as a Microsoft "
----------------
rnk wrote:
> We should add this test case and decide what to do with it:
>   static int x;
>   inline int **f() {
>     static constexpr int *p = true ? 0 : &x;
>     return &p;
>   }
> Currently, in your patch, this diagnostic will come out. MSVC compiles this to guarded, dynamic initialization, despite the constexpr. ;_;
> 
> David thinks we should just give the user the real deal constexpr behavior, even though it's ABI incompatible.
One solution to this would be to create a variation of `evaluateValue()` which activates this new behavior and only use it from `CodeGenModule::EmitConstantInit()`, so that the behavior of `evaluateValue()` in this context doesn't change.  How does that sound?

By the way, I just realized that `CheckPotentialExpressionContainingDeclRefExpr()` eats this diagnostic because of the `SpeculativeLookForDeclRefExprRAII` object.  Should I propagate it up from `CheckPotentialExpressionContainingDeclRefExpr()` to make it user visible?  Right now the test case above only emits "constexpr variable 'p' must be initialized by a constant expression" without any notes.

================
Comment at: lib/AST/ExprConstant.cpp:9008
@@ -8917,1 +9007,3 @@
 
+  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft())
+    InitInfo.useMicrosoftABI();
----------------
rnk wrote:
> This should be limited in scope to only apply to static locals. We should be able to statically initialize globals.
Right, my bad!


http://reviews.llvm.org/D16465