public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches List <gcc-patches@gcc.gnu.org>
Subject: Re: [C++ PATCH] Fix constexpr switch handling (PR c++/77467)
Date: Fri, 16 Sep 2016 20:51:00 -0000	[thread overview]
Message-ID: <20160916204408.GM7282@tucnak.redhat.com> (raw)
In-Reply-To: <CADzB+2ntvwyhcNrJD0S0vdRwSd-EAW79xozKMNj+BVCwKjSRCQ@mail.gmail.com>

On Fri, Sep 16, 2016 at 03:51:11PM -0400, Jason Merrill wrote:
> On Mon, Sep 5, 2016 at 1:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > +  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
> > +     it should be skipped.  E.g. switch (a) b = a;  */
> > +  if (TREE_CODE (body) == STATEMENT_LIST
> > +      || TREE_CODE (body) == BIND_EXPR)
> 
> I'm nervous about this optimization for useless code breaking other
> things that might (one day) wrap a case label; I think I'd prefer to
> drop the condition.

By droping the condition you mean unconditionally call
  cxx_eval_constant_expression (ctx, body, false,
			        non_constant_p, overflow_p, jump_target);
?  That is known not to work, that breaks the
+constexpr int
+bar (int x)
+{
+  int a = x;
+  switch (x)
+    a = x + 1;
+  return a;
+}
handling in the testcase, where body is the MODIFY_EXPR which doesn't have
the label and thus needs to be skipped.  The problem is that all the logic for
skipping statements until the label is found is in cxx_eval_statement_list
only.  For STATEMENT_LIST that is called by cxx_eval_constant_expression,
for BIND_EXPR if we are lucky enough that BIND_EXPR_BODY is a STATEMENT_LIST
too (otherwise I assume even my patch doesn't fix it, it would need to
verify that).  If body is some other statement, then it really should be
skipped, but it isn't, because cxx_eval_constant_expression ignores it.

I wonder if we e.g. cxx_eval_constant_expression couldn't early in the
function for if (*jump_target) return immediately unless code is something
like STATEMENT_LIST or BIND_EXPR with BIND_EXPR_BODY being STATEMENT_LIST,
or perhaps in the future other construct containing other stmts.
I've beeing thinking about TRY block, but at least on the testcases I've
tried it has been rejected in constexpr functions, I think one can't branch
into statement expressions, so that should be fine, OpenMP/OpenACC
constructs are hopefully also rejected in constexpr, what else?

	Jakub

  reply	other threads:[~2016-09-16 20:44 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-05 17:14 Jakub Jelinek
2016-09-16 20:00 ` Jason Merrill
2016-09-16 20:51   ` Jakub Jelinek [this message]
2016-09-19 18:49     ` Jason Merrill
2016-09-20 16:29       ` [C++ PATCH] Fix constexpr switch handling (PR c++/77467, take 2) Jakub Jelinek
2016-09-27 21:33         ` Patch ping Jakub Jelinek
2016-09-28 14:42         ` [C++ PATCH] Fix constexpr switch handling (PR c++/77467, take 2) Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160916204408.GM7282@tucnak.redhat.com \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).