From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46926 invoked by alias); 8 May 2015 20:08:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 46771 invoked by uid 89); 8 May 2015 20:08:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vn0-f49.google.com Received: from mail-vn0-f49.google.com (HELO mail-vn0-f49.google.com) (209.85.216.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 08 May 2015 20:08:47 +0000 Received: by vnbg1 with SMTP id g1so6176972vnb.2 for ; Fri, 08 May 2015 13:08:45 -0700 (PDT) X-Received: by 10.52.231.102 with SMTP id tf6mr4430596vdc.89.1431115724962; Fri, 08 May 2015 13:08:44 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.107.10 with HTTP; Fri, 8 May 2015 13:08:24 -0700 (PDT) In-Reply-To: <5543C6CD.7020403@redhat.com> References: <5543C6CD.7020403@redhat.com> From: Andrew Sutton Date: Fri, 08 May 2015 20:08:00 -0000 Message-ID: Subject: Re: [c++-concepts] code review To: Jason Merrill Cc: gcc-patches List , Braden Obrzut Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-05/txt/msg00747.txt.bz2 Today is the first day I've had to look at these comments. >> if (TEMPLATE_PARM_CONSTRAINTS (current_template_parms)) >> - TYPE_CANONICAL (type) = type; >> + SET_TYPE_STRUCTURAL_EQUALITY (type); > > > This seems like papering over an underlying issue. What was the testcase > that motivated this change? It almost certainly is, but I haven't been able to find or write a minimal test case that reproduces the reason for failure. Basically, we end up with multiple specializations having the same type but different constraints (since constraints are attached to the declaration and not the type itself). I think that I'm running into the same problems with auto a placeholder in recent commits. >> @@ -11854,7 +11854,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t >> complain) >> - if (!spec) >> + if (!spec && DECL_LANG_SPECIFIC (t)) >> - if (!local_p) >> + if (!local_p && DECL_LANG_SPECIFIC (r)) > > > What motivated these changes? From the testcase, it seems that you're > getting here with the decl for "using TD = int", which shouldn't happen. That's the pretty much it... I suppose we could guard against substituting into these kinds of declarations from within tsubst_type_requirement and satisfy_type_constraint. To me it seems like tsubst should work, but just return the same thing. >> @@ -1159,7 +1159,6 @@ check_noexcept_r (tree *tp, int * /*walk_subtrees*/, >> void * /*data*/) >> - tree type = TREE_TYPE (TREE_TYPE (fn)); >> - if (!TYPE_NOTHROW_P (type)) >> + if (!TYPE_NOTHROW_P (TREE_TYPE (fn))) > > > The old code was incorrectly assuming that CALL_EXPR_FN is always a function > pointer, but your new code seems to be incorrectly assuming that it's always > a function or an expression taking the address of a function; I think this > will break on a call to a function pointer variable. I will experiment. >> @@ -3481,13 +3481,27 @@ cxx_eval_constant_expression (const constexpr_ctx >> *ctx, tree t, >> case REQUIRES_EXPR: >> + if (!processing_template_decl) >> + return evaluate_constraint_expression (t, NULL_TREE); >> + else >> + *non_constant_p = true; >> + return t; > > > We shouldn't get here with a dependent REQUIRES_EXPR (or any dependent > expression), so we shouldn't ever hit the else clause. IIRC we get here because of build_x_binary_op. It tries to build non-dependent operands when the operands are not type-dependent. requires-expressions have type bool, so they get run through the constexpr evaluator even when processing_template_decl is true. I've made requires-expressions instantiation dependent, but that doesn't help in this case. >> +static inline bool >> +pending_expansion_p (tree t) >> +{ >> + return (TREE_CODE (t) == PARM_DECL && CONSTRAINT_VAR_P (t) >> + && PACK_EXPANSION_P (TREE_TYPE (t))); >> +} > > > What's the difference between this and function_parameter_pack_p? Not a lot, except that replacing pending_expansion_p in one of the two places that it's used causes ICEs :) This function can almost certainly be removed. >> +check_implicit_conversion_constraint (tree t, tree args, >> + tsubst_flags_t complain, tree >> in_decl) >> +{ >> + tree expr = ICONV_CONSTR_EXPR (t); >> + >> + /* Don't tsubst as if we're processing a template. If we try >> + to we can end up generating template-like expressions >> + (e.g., modop-exprs) that aren't properly typed. */ >> + int saved_template_decl = processing_template_decl; >> + processing_template_decl = 0; > > > Why are we checking constraints when processing_template_decl is true? IIRC I allow constraints to be evaluated in any context because it lets us catch these kinds of errors: template void f() { vector v; // error: constraints not satisfied } >> + ++processing_template_decl; >> + tree constr = transform_expression (lift_function_definition (fn, >> args)); >> + --processing_template_decl; > > > Why do you need to set processing_template_decl here and in other calls to > transform_expression? I don't notice anything that would be helped, > especially now that you're using separate tree codes for constraints, though > there is this in check_logical_expr: > >> + /* Resolve the logical operator. Note that template processing is >> + disabled so we get the actual call or target expression back. >> + not_processing_template_sentinel sentinel. > > > I guess that isn't needed anymore? I've had problems in the past where substitution tries a little too eagerly to fold expressions into constants --- especially type traits. Those need to be preserved in the text for ordering. Although I think this really only matters when you're instantiating a class template whose members are constraints. Andrew