From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24382 invoked by alias); 6 Jun 2013 15:55:29 -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 24371 invoked by uid 89); 6 Jun 2013 15:55:28 -0000 X-Spam-SWARE-Status: No, score=-7.4 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; Thu, 06 Jun 2013 15:55:27 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r56FtQOn019059 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 6 Jun 2013 11:55:26 -0400 Received: from [10.3.113.60] (ovpn-113-60.phx2.redhat.com [10.3.113.60]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r56FtPFc011850; Thu, 6 Jun 2013 11:55:25 -0400 Message-ID: <51B0B0ED.5090508@redhat.com> Date: Thu, 06 Jun 2013 15:55: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: [c++-concepts] code review References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00332.txt.bz2 Hi, I'm finally going through the current code on the branch, sorry for the delay. > * gcc/system.h (cstdlib): Include to avoid poisoned > declaration errors. Poisoned declarations of what? This seems redundant with the #include just below. > + /* Concepts-related keywords */ > + { "assume", RID_ASSUME, D_CXXONLY | D_CXX0X | D_CXXWARN }, > + { "axiom", RID_AXIOM, D_CXXONLY | D_CXX0X | D_CXXWARN }, > + { "concept", RID_CONCEPT, D_CXXONLY | D_CXX0X | D_CXXWARN }, > + { "forall", RID_FORALL, D_CXXONLY | D_CXX0X | D_CXXWARN }, > + { "requires", RID_REQUIRES, D_CXXONLY | D_CXX0X | D_CXXWARN }, I don't see anything that limits these keywords to when concepts are enabled. You probably want to add an additional mask that applies to these. > +; Activate C++ concepts support. > +Variable > +bool flag_concepts You don't need to declare this separately. > Components for process constraints and evaluating constraints. Should that be "processing"? > +// TODO: Simply assinging boolean_type_node to the result type of the expression "assigning" > +// reduced terms in the constraints languaage. Returns NULL_TREE if either A or "language" > +// a constexpr, nullary function teplate whose result can be converted "template" > + // A constraint is declared constexpr Needs a period. > +// This function is not called for abitrary call expressions. In particul, "particular" > +static tree > +resolve_constraint_check (tree ovl, tree args) This function seems to be trying to model a subset of overload resolution, which seems fragile to me; better to use the actual overload resolution code to decide which function the constraint expression calls, or at least resolve_nondeduced_context which handles SFINAE. > + case CAST_EXPR: > + return reduce_node (TREE_VALUE (TREE_OPERAND (t, 0))); Are we assuming that constraint expressions can't involve objects of literal class type? > +// If T is a call to a constraint instantiate it's definition and "its" > + tree c = finish_call_expr (t, &args, true, false, 0); > + error ("invalid requirement"); > + inform (input_location, "did you mean %qE", c); For both of these diagnostics, let's use EXPR_LOC_OR_HERE (t) as the location. > +// Reduce the requirement T into a logical formula written in terms of > +// atomic propositions. > +tree > +reduce_requirements (tree reqs) s/T/REQS/ > struct GTY(()) tree_template_info { > struct tree_common common; > + tree constraint; > vec *typedefs_needing_access_checking; > }; Why do we need constraint information in template_info? I suppose this is the issue you raised in your mail last month: > In general constraints are directly associated with a template decl. For example: > > template > class complex; > > The Arithmetic constraint is associated with the template decl. However, this doesn't seem to work with partial specializations: > > template > struct complex { ... }; > > I had expected there to be a template decl associated with underlying class, but after print-bombing most of the instantiation, lookup, and specialization processing routines, I couldn't find that one was ever created for the type decl. This does seem like a shortcoming, that also led to the typedefs vec getting stuck into the template_info inappropriately. I guess we should start building TEMPLATE_DECLs for partial specializations. > +/* Constraint information for a C++ declaration. This includes the > + requirements (as a constant expression) and the decomposed assumptions > + and conclusions. The assumptions and conclusions are cached for the > + purposes of overlaod resolution and diagnostics. */ > +struct GTY(()) tree_constraint_info { > + struct tree_base base; > + tree spelling; > + tree requirements; > + tree assumptions; > +}; I'm confused by the relationship between the comment and the field names here. Where do the conclusions come in? Is "requirements (as a constant expression)" in the spelling or requirements field? Also, "overload". > +constraint_info_p (tree t) > +template_info_p (tree t) Let's use check_* rather than *_p for these, too. > +// NODE must be a lang-decl. Let's say "NODE must have DECL_LANG_SPECIFIC" to avoid confusion with struct lang_decl. > + error ("concept %q#D declared with function arguments", fn); s/arguments/parameters/. Some of the gcc internals get this distinction wrong; but we don't need to expose that in diagnostics... > + // If the concept declaration specifier was found, check > + // that the declaration satisfies the necessary requirements. > + if (inlinep & 4) > + { > + DECL_DECLARED_CONCEPT_P (decl) = true; > + if (!check_concept_fn (decl)) > + return NULL_TREE; > + } I think I'd rather deal with an invalid concept by not marking it as a concept, but still declaring it as a constexpr function. > + flag_concepts = true; This is redundant since c.opt specifies that it defaults to true. > +// Return the current conclision. "conclusion" > +// Return the current list of assumed terms. > +inline term_list & > +assumptions (proof_state &s) { return assumptions (current_goal (s)); } Just as a comment, this use of internal iterators and forwarding functions seems confusing; a reader is likely to think that assumptions (s) would produce a list of assumptions for the state as a whole, rather than for the current goal. It's particularly surprising in functions like decompose_left_goal, where we look at the innermost iterator and then pass the outermost object to a subroutine. > +// The following functions are used to manage the insertation and removal > +// goals and terms in the proof state. "insertion and removal of" > +// the new objet. This does not update the current goal. "object" > +// And right logical rule. > +inline void > +and_right (proof_state &s) > +{ > + gcc_assert (TREE_CODE (conclusion (s)) == TRUTH_ANDIF_EXPR); > + tree t = ignore_term (s); > + conclude_term (branch_goal (s), TREE_OPERAND (t, 1)); > + conclude_term (current_goal (s), TREE_OPERAND (t, 0)); > +} I believe the intent here is to split a sequent A |- B ^ C into two, A |- B A |- C right? But since branch_goal updates the current sub-goal, I would expect the result to be A |- A |- C, B instead. And likewise for or_left. > +// a tree (a vetor of vectors). "vector" > +// Extract a vector of term vectors from s. The selected set of terms is given > +// by the projection function proj. This is generally either assumptions or > +// conclusions. > +template > + tree > + extract_goals (proof_state& s, F proj) Why is proj a function argument rather than a template argument, which would allow inlining? > +// Decompose the requirement R into a set of conclusionss, returning a "conclusions" > +decompose_conclusions (tree r) This function is currently never used; I assume that is planned for a later patch? > +// Returns true if the list of assumptions AS subsume the atomic > +// proposition C. This is the case when we can find a proposition in > +// HS that entails the conclusion C. s/HS/AS/ > + return subsumes_prop (as, TREE_OPERAND (c, 0)) > + && subsumes_prop (as, TREE_OPERAND (c, 1)); > + case TRUTH_ORIF_EXPR: > + return subsumes_prop (as, TREE_OPERAND (c, 0)) > + || subsumes_prop (as, TREE_OPERAND (c, 1)); Multi-line expressions need parentheses. > +// The following functions will optionally parse a rule if the corresponding > +// criteria is satisfied. If not, the parser returns NULL indicating that > +// the optional parse taken. Missing a "was not". And these functions should also take the rule as a template argument rather than a function argument. And why do it this way rather than check and possibly return at the top of the function, as elsewhere in the parser? You already have cp_parser_requires_clause checking for RID_REQUIRES. > - expect the specialization handler to detect and report this. */ > + expect the specialization handler to detect and report this. */ There are a bunch of unnecessary whitespace changes in parser.c, including this one. > @@ -19773,6 +19883,17 @@ cp_parser_member_declaration (cp_parser* parser) > else > initializer = NULL_TREE; > > + // If we're looking at a function declaration, then a requires > + // clause may follow the declaration. > + tree saved_template_reqs = current_template_reqs; Why don't you use 'release' and conjoin_requirements here? > +// Try to substitute ARGS into PARMS, returning the actual list of > +// arguments that have been substituted. If ARGS cannot be substituted, > +// return error_mark_node. > +tree > +substitute_template_parameters (tree parms, tree args) The comment sounds more like tsubst_template_parms than coerce_template_parms. Jason