From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21896 invoked by alias); 15 Jun 2013 01:40:39 -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 21886 invoked by uid 89); 15 Jun 2013 01:40:39 -0000 X-Spam-SWARE-Status: No, score=-7.3 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sat, 15 Jun 2013 01:40:38 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r5F1eaFo025969 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 14 Jun 2013 21:40:36 -0400 Received: from [10.3.113.28] (ovpn-113-28.phx2.redhat.com [10.3.113.28]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r5F1eZ2j016869; Fri, 14 Jun 2013 21:40:35 -0400 Message-ID: <51BBC613.5040708@redhat.com> Date: Sat, 15 Jun 2013 01:40:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Thunderbird/23.0a2 MIME-Version: 1.0 To: Andrew Sutton CC: Gabriel Dos Reis , gcc-patches@gcc.gnu.org Subject: Re: [c++-concepts] code review References: <51B0B0ED.5090508@redhat.com> <51B0F122.6020301@redhat.com> <51B62961.1080409@redhat.com> <51B8A36C.1080005@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00876.txt.bz2 > 1. tree_template_info still contains constraint_info. That will change > in a subsequent patch. I'm kind of waiting for specializations to get > TEMPLATE_DECLs. Makes sense. > 2. I left the include in system.h, because removing it will > result in errors due to an inclusion of . It's probable > that both can be removed, but I didn't test it with this patch. You mean you don't need anymore in logic.cc? I think we want the include in general if we're going to support people using the C++ standard library. > + if (DECL_USE_TEMPLATE (fn)) > + { > + tree cons = DECL_TEMPLATE_CONSTRAINT (fn); > + if (!check_constraints (cons)) > + { > + // TODO: Fail for the right reason. > + reason = template_unification_error_rejection (); Can't you do template_constraint_failure (DECL_TI_TEMPLATE (fn), DECL_TI_ARGS (fn)) ? > if (fn == error_mark_node) > { > + if (!check_template_constraints (tmpl, targs)) > + reason = template_constraint_failure (tmpl, targs); > + else if (errorcount+sorrycount == errs) > /* Don't repeat unification later if it already resulted in errors. */ A constraint failure is a form of unification failure, like SFINAE; let's handle it in that context rather than separately here, and reserve rr_constraint_failure for the case of a non-template member function. > +// Returns the template declaration associated with the candidate > +// function. For actual templates, this is directly associated > +// with the candidate. For temploids, we return the template > +// associated with the specialization. > +static inline tree > +get_actual_template (struct z_candidate *cand) The uses of "actual template" in the comment and function name seem to mean different things. Maybe call the function template_decl_for_candidate? > + // Non-temploids cannot be constrained. Can friend temploids be constrained? > + if (!DECL_TEMPLOID_INSTANTIATION (new_fn) && > + !DECL_TEMPLOID_INSTANTIATION (old_fn)) The && goes at the beginning of the second line. > +// reduced terms in the constraints language. Returns NULL_TREE if either A or > // B are NULL_TREE. > tree > conjoin_requirements (tree a, tree b) > { > - if (a && b) > - return build_min (TRUTH_ANDIF_EXPR, boolean_type_node, a, b); > + if (a) > + return b ? join_requirements (TRUTH_ANDIF_EXPR, a, b) : a; > + else if (b) > + return b; > else > return NULL_TREE; Seems like the comment is no longer accurate; the function now only returns NULL_TREE if both a and b are NULL_TREE. > +// Returns true if the function decl F is a constraint predicate. It must be a > +// constexpr, nullary function with a boolean result type. > +static tree > +get_constraint_check (tree call) The comment needs updating. And the function isn't used anywhere; it seems redundant with resolve_constraint_check. > +// The raison d'entre of this class is to prevent traits "d'etre" > +// and other template nodes from generating new expressions > +// during instantiation. I'm not clear on the issue. Perhaps leaving processing_template_decl alone and using fold_non_dependent_expr would accomplish what you want? > +static inline bool > +check_type_constraints (tree t, tree args) > +{ > + return check_constraints (CLASSTYPE_TEMPLATE_CONSTRAINT (t), args); > +} > + > +static inline bool > +check_decl_constraints (tree t, tree args) > +{ > + if (TREE_CODE (t) == TEMPLATE_DECL) > + return check_decl_constraints (DECL_TEMPLATE_RESULT (t), args); > + else > + return check_constraints (DECL_TEMPLATE_CONSTRAINT (t), args); > +} > + > +// Check the template's constraints for the specified arguments, > +// returning true if the constraints are satisfied and false > +// otherwise. > +bool > +check_template_constraints (tree t, tree args) > +{ > + return check_constraints (get_constraints (t), args); > +} Seems like the last function would also work for types and non-template decls. > + error_at (loc, "constraints not satisfied"); I assume you're planning to make this more verbose? It could use a TODO to that effect. > +// and template isntantiation. Evaluation warnings are also inhibited. "instantiation" > + else if (are_constrained_overloads (newdecl, olddecl)) > + { > + // If newdecl and olddecl are function templates with the > + // same type but different constraints, then they cannot be > + // duplicate declarations. However, if the olddecl is declared > + // as a concept, then the program is ill-formed. > + if (DECL_DECLARED_CONCEPT_P (DECL_TEMPLATE_RESULT (olddecl))) > + { > + error ("cannot specialize concept %q#D", olddecl); > + return error_mark_node; > + } > + return NULL; > + } We should check for differing constraints in decls_match, and then this code should be in the !types_match section of duplicate_decls. > error ("concept %q#D result must be convertible to bool", fn); Why don't we require the result to actually be bool? It seems difficult to decompose dependent requirements if the return type can be something else. > +// Remove the current term form the list, repositioning to the term "from" > +// \Gamma, P |- Delta \Gamma, Q |- \Delta Are the \ for TeX markup or something? You're missing one on the left Delta here. BTW, thanks: I find the rewritten logic code more readable. > +subsumes_constraints (tree left, tree right) > +{ > + gcc_assert (check_constraint_info (left)); > + gcc_assert (check_constraint_info (right)); > + > + // Check that c is subsumed by each subproblem in h. > + // If it is not, then h does not subsume c. > + tree as = CI_ASSUMPTIONS (left); > + tree c = reduce_requirements (CI_REQUIREMENTS (right)); > + for (int i = 0; i < TREE_VEC_LENGTH (as); ++i) > + if (!subsumes_prop (TREE_VEC_ELT (as, i), c)) > + return false; > + return true; "h" doesn't appear anywhere in the function; let's change the comment to refer to "left" and "right" instead. > + cp_lexer_next_token_is_keyword (parser->lexer, RID_REQUIRES)) > { > tree reqs = release (current_template_reqs); > - if (tree r = cp_parser_requires_clause_opt (parser)) > + if (tree r = cp_parser_requires_clause (parser)) I was thinking to change the name of cp_requires_clause to cp_parser_requires_clause_opt and change the assert to a test for the keyword and return NULL_TREE if it isn't found. > - if (flag_concepts > - && function_declarator_p (declarator) > + if (flag_concepts && > + function_declarator_p (declarator) && The && was in the right place before this change. > + if (!potential_rvalue_constant_expression (expr)) > + { > + require_potential_rvalue_constant_expression (expr); This can just be if (!require_pot.... > + // FIXME: we should be checking the constraints, not just > + // instantiating them. Let's check them in determine_specialization, along with the SFINAE check after the comment /* Make sure that the deduced arguments actually work. */ Can explicit specializations have constraints, to indicate which template they are specializing? > + // FIXME: This points to the wrong line. > + error_at (input_location, "redeclaration %q+D with different constraints", tmpl); %q+D is telling the diagnostic code to use DECL_SOURCE_LOCATION (tmpl) as the location. I think you want error_at (input_location, "redeclaration of %q#D with different " "constraints", tmpl); inform (DECL_SOURCE_LOCATION (tmpl), "original declaration appeared here"); > + tree argcons = TREE_CODE (argtype) == TEMPLATE_TEMPLATE_PARM > + ? DECL_TEMPLATE_CONSTRAINT (DECL_TEMPLATE_RESULT (parm)) > + : TYPE_TEMPLATE_CONSTRAINT (argtype); Seems like get_constraints would be useful here. > + error ("constraint mismatch at argument %d in " > + "template parameter list for %qD", > + i + 1, in_decl); Let's also print the parameter and argument. > +// Argument deduction ios applied to all template arguments, and Passing 'true' for require_all_args means no deduction will be done; rather, all arguments must either be specified or have default arguments. > + // Check class template requirements. Note that constraints will > + // already have been checked when trying to find a most specialized > + // class among multiple specializations. I don't see where that is; more_specialized_class doesn't seem to compare constraints yet. > + tree cons = DECL_CLASS_TEMPLATE_P (t) > + ? TYPE_TEMPLATE_CONSTRAINT (type) > + : DECL_TEMPLATE_CONSTRAINT (decl); get_constraints would be useful here as well. > + // FIXME: Do we need to attach constraints? > + // TODO: Is this actually necessary? I don't think you need to fill in TI_CONSTRAINT for anything but the main template patterns, here or other places. > +substitute_requirements (tree reqs, tree args) This seems redundant with instantiate_requirements. > + // Build template info and associate it with Incomplete sentence. > + // TODO: Do we need to propagate constaints into bound template > + // template parameters? Probably not for interface checking. "constraints" But I agree probably not, for the same reason we don't need constraints on instantiated FIELD_DECLs. Jason