Hi!

Thanks for the review, let me start with the unrelated bugfixes then and
deal with the rest later.

On Tue, Oct 01, 2019 at 05:56:00PM -0400, Jason Merrill wrote:
> > @@ -3905,7 +4033,7 @@ cxx_eval_store_expression (const constex
> >     bool no_zero_init = true;
> >     releasing_vec ctors;
> > -  while (!refs->is_empty())
> > +  while (!refs->is_empty ())
> >       {
> >         if (*valp == NULL_TREE)
> >   	{
> > @@ -4046,7 +4174,9 @@ cxx_eval_store_expression (const constex
> >     if (const_object_being_modified)
> >       {
> >         bool fail = false;
> > -      if (!CLASS_TYPE_P (TREE_TYPE (const_object_being_modified)))
> > +      tree const_objtype
> > +	= strip_array_types (TREE_TYPE (const_object_being_modified));
> > +      if (!CLASS_TYPE_P (const_objtype))
> 
> This looks like an unrelated bugfix; you might commit it (and the others)
> separately if that's convenient.
> 
> > @@ -4907,14 +5043,21 @@ cxx_eval_constant_expression (const cons
> >         break;
> >       case CLEANUP_STMT:
> > -      r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
> > +      {
> > +	tree initial_jump_target = jump_target ? *jump_target : NULL_TREE;
> > +	r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval,
> > +					  non_constant_p, overflow_p,
> > +					  jump_target);
> > +	if (!CLEANUP_EH_ONLY (t) && !*non_constant_p)
> > +	  /* Also evaluate the cleanup.  If we weren't skipping at the
> > +	     start of the CLEANUP_BODY, change jump_target temporarily
> > +	     to &initial_jump_target, so that even a return or break or
> > +	     continue in the body doesn't skip the cleanup.  */
> 
> This also looks like an unrelated bugfix.

Both are only partially related, I think they can go in separately, but at
least for the first one I haven't managed to come up with a testcase where
it would matter and nothing in e.g. check-c++-all comes there with ARRAY_TYPE,
is it ok for trunk without a testcase (first attached patch)?

As for the second bugfix, I think it should be accompanied with the
potential_constant_expression_1 change, and there I've actually managed to
come up with a testcase where it matters, though it is using a GCC extension
(generally, CLEANUP_STMTs won't appear in constexpr functions that often
because of the requirement that variables in them have literal type and
literal types have trivial destructors, so really no cleanups in that case).

The testcase where it makes a difference is:

constexpr void
cleanup (int *x)
{
  if (x)
    asm ("");
}

constexpr void
cleanup2 (int *x)
{
}

constexpr bool
foo ()
{
  int a __attribute__((cleanup (cleanup))) = 1;
  return true;
}

constexpr bool
bar ()
{
  int a __attribute__((cleanup (cleanup2))) = 1;
  return true;
}

constexpr auto x = foo ();
constexpr auto y = bar ();

With vanilla trunk, one gets a weird message:
test.C:27:24: error: ‘constexpr bool foo()’ called in a constant expression
   27 | constexpr auto x = foo ();
      |                    ~~~~^~
test.C:28:24: error: ‘constexpr bool bar()’ called in a constant expression
   28 | constexpr auto y = bar ();
      |                    ~~~~^~
That is because we call potential_constant_expression_1 on the foo (or
bar) body, see CLEANUP_STMT in there and punt.  With just the
potential_constant_expression_1 change to handle CLEANUP_STMT, we actually
accept it, which is wrong.
Finally, with the whole patch attached below, we reject foo () call and
accept bar ():
test.C:27:24:   in ‘constexpr’ expansion of ‘foo()’
test.C:27:25:   in ‘constexpr’ expansion of ‘cleanup((& a))’
test.C:5:5: error: inline assembly is not a constant expression
    5 |     asm ("");
      |     ^~~
test.C:5:5: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a

Is the testcase ok in the second patch?  Are those patches ok for trunk?

	Jakub