From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 952693861012 for ; Thu, 10 Jun 2021 19:00:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 952693861012 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-451-cDwFQH16Nbuk8momK-gl1A-1; Thu, 10 Jun 2021 15:00:57 -0400 X-MC-Unique: cDwFQH16Nbuk8momK-gl1A-1 Received: by mail-qt1-f200.google.com with SMTP id z4-20020ac87f840000b02902488809b6d6so472292qtj.9 for ; Thu, 10 Jun 2021 12:00:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=N8uy3Z5BmxSJ55GQx6Nk6WThrgdw+71yJCJku5SgI5o=; b=VSwwBEXWlejjubbAHPa0CAcGLBzqamkA2D5rUSuRTtkhMPohIgmta9t5JT58rcIEv0 Ig2letvGAjfV9zzNUD2g77w8z0GijO5bzzz4sF+MRsEF6rdU+u+fn3PtbYRAxAoWujuT irXDRS3hBvIgzTMCdN8/4fdUGjk9Dq3v541dWGcKXGY1IQH5oUZmBy4oyRHWWsKhWgqf Z70eaapwIDShtmEeSRy3wws8K9gi5HZn7DkL/vAnT89agxdMihIESQ8sIlIBYKyeWtZQ nZ7rf663bfIkCJkSXM6hXTHlFabE8+fqFwr+T3jjHLhfvNlFHwCD0MX2nYC47sL/NNJj I8Rw== X-Gm-Message-State: AOAM533PKn9pYwkCOxAMCCJlCFy4mgFM9OQhTbyIjrhVBnQQ+6MSpMJD Ioqh6NDVE7qneKnHJf/uQL5WMLUl9Y2HkBgekEShmP+k6tbW/gHBDQ33MLTwlSjjl8TJvApFVib WSvHXEEBxI4jAq0vwRw== X-Received: by 2002:ac8:12ca:: with SMTP id b10mr283147qtj.108.1623351656490; Thu, 10 Jun 2021 12:00:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwjRUsAcGbFz9k0LgTFYEOXXgV1LAo5CQ7vtnH5ETXuaJKduKwiE4DU34WTkDYPEw0dca0dsg== X-Received: by 2002:ac8:12ca:: with SMTP id b10mr283106qtj.108.1623351656144; Thu, 10 Jun 2021 12:00:56 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id 2sm2746117qtb.28.2021.06.10.12.00.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Jun 2021 12:00:55 -0700 (PDT) Subject: Re: [PATCH] c++: Add C++23 consteval if support - P1938R3 [PR100974] To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org, Jonathan Wakely References: <20210610083416.GC7746@tucnak> <20210610144408.GH7746@tucnak> From: Jason Merrill Message-ID: Date: Thu, 10 Jun 2021 15:00:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210610144408.GH7746@tucnak> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jun 2021 19:01:00 -0000 On 6/10/21 10:44 AM, Jakub Jelinek wrote: > On Thu, Jun 10, 2021 at 10:09:26AM -0400, Jason Merrill wrote: >>> --- gcc/cp/parser.c.jj 2021-06-09 21:54:39.482193853 +0200 >>> +++ gcc/cp/parser.c 2021-06-10 10:09:23.753052980 +0200 >>> @@ -10902,6 +10902,11 @@ cp_parser_lambda_expression (cp_parser* >>> bool discarded = in_discarded_stmt; >>> in_discarded_stmt = 0; >>> + /* Similarly the body of a lambda in immediate function context is not >>> + in immediate function context. */ >>> + bool immediate_fn_ctx_p = in_immediate_fn_ctx_p; >> >> It's hard to distinguish between these two names when reading the code; >> let's give the local variable a name including "saved". > > Will do. > >>> + if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) >>> + error ("% requires compound statement"); >>> + >>> + in_immediate_fn_ctx_p |= ce > 0; >>> + cp_parser_implicitly_scoped_statement (parser, NULL, guard_tinfo); >> >> Maybe use cp_parser_compound_statement directly instead of this and checking >> CPP_OPEN_BRACE above? Either way is fine. > > I thought doing it this way will provide better diagnostics for what I think > can be a common bug - people so used to normal if not requiring compound > statements forgetting those {}s from time to time. > What the patch currently diagnoses is: > consteval-if2.C:13:6: error: ‘if consteval’ requires compound statement > 13 | if consteval if (true) {} // { dg-error "'if consteval' requires compound statement" } > | ^~~~~~~~~ > while with cp_parser_compound_statement directly it diagnoses: > consteval-if2.C:13:16: error: expected ‘{’ before ‘if’ > 13 | if consteval if (true) {} // { dg-error "'if consteval' requires compound statement" } > | ^~ > What do you prefer? The second is clearer about the fix, the first is clearer about the problem. Maybe add a fixit to the first error? > Dunno if the fine detail that in the grammar only one of the statements > is compound-statement and then there is a requirement that the other > statement has to be a compound-statement shouldn't affect how it is > reported. The difference was to prevent "else ;" from binding to an enclosing if rather than the if consteval. >>> + if (TREE_CODE (t) == IF_STMT && IF_STMT_CONSTEVAL_P (t)) >>> + { >>> + /* Evaluate the condition as if it was >>> + if (__builtin_is_constant_evaluated ()). */ >>> + if (ctx->manifestly_const_eval) >>> + val = boolean_true_node; >>> + else >>> + { >>> + *non_constant_p = true; >>> + return t; >>> + } >> >> Why set *non_constant_p? Shouldn't this just be >> >> val = boolean_false_node; >> >> so we constant-evaluate the else clause when we are trying to >> constant-evaluate in a non-manifestly-constant-evaluated context? > > It matches what we do for CP_BUILT_IN_IS_CONSTANT_EVALUATED calls: > /* For __builtin_is_constant_evaluated, defer it if not > ctx->manifestly_const_eval, otherwise fold it to true. */ > if (fndecl_built_in_p (fun, CP_BUILT_IN_IS_CONSTANT_EVALUATED, > BUILT_IN_FRONTEND)) > { > if (!ctx->manifestly_const_eval) > { > *non_constant_p = true; > return t; > } > return boolean_true_node; > } > I believe we sometimes try to constexpr evaluate something without > having manifestly_const_eval = true even on expressions that > are in manifestly constant evaluated contexts and am worried if > we just folded it to boolean_false_node we could get a constant expression > and replace the expression with that, even before we actually try to > constant evaluate it with manifestly_const_eval = true. > > If I do (for the CP_BUILT_IN_IS_CONSTANT_EVALUATED): > --- gcc/cp/constexpr.c.jj 2021-06-10 15:27:31.123353594 +0200 > +++ gcc/cp/constexpr.c 2021-06-10 16:26:38.368168281 +0200 > @@ -1320,10 +1320,7 @@ cxx_eval_builtin_function_call (const co > BUILT_IN_FRONTEND)) > { > if (!ctx->manifestly_const_eval) > - { > - *non_constant_p = true; > - return t; > - } > + return boolean_false_node; > return boolean_true_node; > } > > then > FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++14 execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++17 execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++2a execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++17 -fconcepts execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++14 execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++17 execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++2a execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++17 -fconcepts execution test > FAIL: g++.dg/cpp2a/is-constant-evaluated9.C -std=gnu++2a (test for excess errors) > at least regresses. OK, just add a comment then. >>> + if (TREE_CODE (t) != IF_STMT || !IF_STMT_CONSTEVAL_P (t)) >>> + { >>> + if (integer_zerop (tmp)) >>> + return RECUR (TREE_OPERAND (t, 2), want_rval); >>> + else if (TREE_CODE (tmp) == INTEGER_CST) >>> + return RECUR (TREE_OPERAND (t, 1), want_rval); >>> + } >> >> Don't we still want to shortcut consideration of one of the arms for if >> consteval? > > potential_constant_expression_p{,_1} doesn't get passed whether it is > manifestly_const_eval or not. OK, just add a comment then. >>> --- gcc/cp/cp-gimplify.c.jj 2021-06-09 21:54:39.473193977 +0200 >>> +++ gcc/cp/cp-gimplify.c 2021-06-10 09:49:35.898557178 +0200 >>> @@ -161,7 +161,9 @@ genericize_if_stmt (tree *stmt_p) >>> if (!else_) >>> else_ = build_empty_stmt (locus); >>> - if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_)) >>> + if (IF_STMT_CONSTEVAL_P (stmt)) >>> + stmt = else_; >> >> This seems redundant, since you're using boolean_false_node for the >> condition. > > It is only when !TREE_SIDE_EFFECTS (else_). > I think that is about having labels in the then_/else_ blocks and > gotos jumping into those from outside. > But IF_STMT_CONSTEVAL_P guarantees that is not the case (and we verify > earlier), so we can do that (and in fact, for IF_STMT_CONSTEVAL_P have > to, because then_ could contain consteval calls not constant expression > evaluated). > I guess we could do that for IF_STMT_CONSTEXPR_P too, that also > doesn't allow gotos/switch into the branches. > >>> --- gcc/cp/call.c.jj 2021-06-09 21:54:39.436194489 +0200 >>> +++ gcc/cp/call.c 2021-06-10 09:49:35.949556470 +0200 >>> @@ -8840,6 +8840,7 @@ immediate_invocation_p (tree fn, int nar >>> || !DECL_IMMEDIATE_FUNCTION_P (current_function_decl)) >>> && (current_binding_level->kind != sk_function_parms >>> || !current_binding_level->immediate_fn_ctx_p) >>> + && !in_immediate_fn_ctx_p >> >> Now that we have this flag, shouldn't we set it in actual immediate function >> context? Or rename the flag to match when it's actually set. > > I guess I can try that. Though, I'm not sure if we could also get rid of > the current_binding_level->immediate_fn_ctx_p for sk_function_parms case. > > Jakub >