[PATCH] [Parse] Allow 'constexpr' in condition declarations

[Richard Smith]

On Fri, May 1, 2015 at 11:28 AM, Meador Inge <meadori at gmail.com> wrote:

> Opening up in **constexpr** s in conditions allows "maybe" odr used
> expression to be collected in cases like:
>
>   class T {
>   public:
>      constexpr T(int v) : v(v) { }
>      constexpr operator int() const { return v; }
>   private:
>      int v;
>   };
>
>   int main() {
>      if (constexpr T x = 200) { }
>   }
>
> Without the cleanup the following assert fires:
>
>   (MaybeODRUseExprs.empty() && "Leftover expressions for odr-use
> checking"), function ActOnFinishFunctionBody, file
> /Users/meadori/Code/source/llvm/tools/clang/lib/Sema/SemaDecl.cpp, line
> 10820.
>
> **MaybeODRUseExprs** is populated in **DoMarkVarDeclReferenced** from
> **SemaExpr.cpp** (the end of the function does an explicit check for
> constant expressions).  I don't think this situation could ever happen for
> conditional declarations before,


OK, I think you're right. The interesting thing here is that a condition
variable seems to involve two separate full-expressions: one for the
initializer and one for the reference to the condition variable (and --
except for 'switch' -- for the contextual conversion to bool). We are
missing the ActOnFinishFullExpr call for the second full-expression.

Now, it just so happens that nothing we do in ActOnFinishFullExpr for the
condition expression is observable except for resolving potential odr-uses,
and without constexpr conditions, every potential use is a real odr-use, so
has already been handled.

I think the right thing to do here is to pass the expression produced by
loading and possibly converting the condition variable to
ActOnFinishFullExpr, rather than calling MaybeCreateExprWithCleanups.

but know it can and we need the new cleanup.
>
>
> ================
> Comment at: include/clang/Parse/Parser.h:1760
> @@ -1760,1 +1759,3 @@
> +                                   DeclSpecContext DSC = DSC_normal,
> +                                   bool AllowConstexprs = false);
>
> ----------------
> rsmith wrote:
> > Please add a new DSC value for conditions rather than adding a flag here.
> Will fix.
>
> ================
> Comment at: lib/Sema/SemaExprCXX.cpp:2585
> @@ -2584,1 +2584,3 @@
>
> +  Condition = MaybeCreateExprWithCleanups(Condition.get());
> +
> ----------------
> rsmith wrote:
> > Do your tests cover the need for this?
> Not yet, but I will add a test for it.
>
> ================
> Comment at: test/CXX/stmt.stmt/stmt.select/p6.cpp:4-5
> @@ +3,4 @@
> +
> +// This test verifies the functionality specified by DR948:
> +// http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#948
> +
> ----------------
> rsmith wrote:
> > The right place for such a test is tests/CXX/drs/dr9xx.cpp.
> Will fix.
>
> http://reviews.llvm.org/D8978
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150501/e2b6833e/attachment.html>