| Message ID | d9f3ed05-06c4-166b-6ac2-2fe573c99297@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 71140 ("[concepts] ill-formed nested-requirement lacking a semicolon not rejected") | expand |
Hi, already pinging this because it seems rather straightforward to me... On 03/10/18 14:18, Paolo Carlini wrote: > Hi, > > a simple issue, we weren't correctly implementing 7.5.7.4 on the > terminating semicolon. Tested x86_64-linux. https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00173.html Thanks! Paolo.
On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote: > a simple issue, we weren't correctly implementing 7.5.7.4 on the > terminating semicolon. Tested x86_64-linux. If the missing semicolon is followed by }, let's allow it with a pedwarn. Jason
Hi, On 11/10/18 20:36, Jason Merrill wrote: > On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote: >> a simple issue, we weren't correctly implementing 7.5.7.4 on the >> terminating semicolon. Tested x86_64-linux. > If the missing semicolon is followed by }, let's allow it with a pedwarn. I see. Unfortunately we have yet another issue in this area: our requirement-list, as parsed in cp_parser_requirement_list, isn't the same as requirement-seq in the working draft: requirement-list: requirement requirement-list ';' requirement[opt] vs requirement-seq requirement requirement-seq requirement thus, in particular, we accept a single requirement either terminated with semicolon or not (which explains why we have c++/71139 and c++/71140 es accept invalid but we don't reject anything valid). We do this together with correctly enforcing the terminating semicolon for simple-requirement and type-requirement and not enforcing it for compound-requirement and nested-requirement (per the bugs at issue). Sort of a mess. I don't know how much we care about backward compatibility with the TS, etc, in this area (*) but it would be *so* nice to implement requirement-seq too correctly and simply require the terminating semicolon for all the 4 kinds of requirements... Thanks, Paolo. (*) In principle we could even imagine a legacy Concepts TS mode - by and large frozen, the way of the library TR1 - and a proper C++20 concepts mode, useful for much more serious issues too. No idea if somebody already discussed this?!?
On Thu, Oct 11, 2018 at 4:31 PM Paolo Carlini <paolo.carlini@oracle.com> wrote: > > On 11/10/18 20:36, Jason Merrill wrote: > > On Wed, Oct 3, 2018 at 8:18 AM Paolo Carlini <paolo.carlini@oracle.com> wrote: > >> a simple issue, we weren't correctly implementing 7.5.7.4 on the > >> terminating semicolon. Tested x86_64-linux. > > If the missing semicolon is followed by }, let's allow it with a pedwarn. > > I see. Unfortunately we have yet another issue in this area: our > requirement-list, as parsed in cp_parser_requirement_list, isn't the > same as requirement-seq in the working draft: > > requirement-list: > requirement > requirement-list ';' requirement[opt] > > vs > > requirement-seq > requirement > requirement-seq requirement > > thus, in particular, we accept a single requirement either terminated > with semicolon or not (which explains why we have c++/71139 and > c++/71140 es accept invalid but we don't reject anything valid). We do > this together with correctly enforcing the terminating semicolon for > simple-requirement and type-requirement and not enforcing it for > compound-requirement and nested-requirement (per the bugs at issue). > Sort of a mess. I don't know how much we care about backward > compatibility with the TS, etc, in this area (*) but it would be *so* > nice to implement requirement-seq too correctly and simply require the > terminating semicolon for all the 4 kinds of requirements... And allowing a missing semicolon at the end of the list/sequence (with a pedwarn) smooths over the difference. > (*) In principle we could even imagine a legacy Concepts TS mode - by > and large frozen, the way of the library TR1 - and a proper C++20 > concepts mode, useful for much more serious issues too. No idea if > somebody already discussed this?!? That seems like a lot of hassle for little use; we can't just put it in a different file like the library can, and users of concepts will want to the up-to-date implementation. If someone wants to compare against the current TS implementation, they can use GCC 8. BTW, I would discourage you from messing much with the concepts code at this point, as a major overhaul should be coming soon. Jason
> > BTW, I would discourage you from messing much with the concepts code > at this point, as a major overhaul should be coming soon. > Major overhaul: https://github.com/asutton/gcc (branch is concepts; we're about 2 weeks back from trunk). Unfortunately, I we haven't been following GCC commit discipline, and there's a bunch of dead/legacy code that I need to clean up. And of course some regressions. I wanted to spend the weekend on this and forward a cleaner version next week. But no time is as good as the present it seems. This fork reimplements concepts as currently specified in the WD (for the most part). It also preserves TS syntax (but not behavior), although there are certainly going to be some new bugs. -std=c++2a turns on concepts by default (sets -fconcepts) -fconcepts-ts can be additionally specified to enable TS extensions (abbreviated fn templates, etc). -fconcepts on its own gives you (should give you) TS syntax with C++20 semantics and no C++20 features. Here's what's changed: - new requires clause syntax as required in the WD (-fconcepts-ts will change this back to the TS syntax) - concept bool is now a warning, although (IIRC) disabled with -fconcepts-ts. Function and variable concepts live on. - concepts are now their own kind of declaration (CONCEPT_DECL). That was a big change. - now only 3 kinds of constraints: conj, disj, and pred (should be atomic, also needs a dead code cleanup). - constraints on declarations are represented as expressions -- no normalization until later - associated constraints are only instantiated when checked -- no premature substitution - new implementaiton of satisfaction, does not require ahead-of-time normalization - new implementation of normalization (fewer nodes, smaller impl) - atomic constraint comparison based on expr identity/parameter mapping - complete rewrite of subsumption (new comparison model invalidated some assumptions in the old impl) - constrained decls differentiated by syntax of constraints (not equivalence) - moved the concepts testsuite into c++2a directory as a vetting/curating process, new 2a tests, new ts-compatability tests There are some bugs and regressions. I know for a fact that we've broken partial specialization of variable templats, but I'm really not sure how. There's also probably a bug in the constraint comparison implementation that affects partial ordering. More testing is needed. I mostly ignored the TS support while updating to the WD semantics, so that's been a little buggy when I brought it back online. Also, sometimes diagnostics aren't emitted correctly. I'm not quite sure how to proceed with submitting this patch. Once I made the decision to make concepts their own kind of declaration, the idea of sending small patches went right out the window.
Hi, On 12/10/18 14:55, Jason Merrill wrote: > BTW, I would discourage you from messing much with the concepts code > at this point, as a major overhaul should be coming soon. Agreed: we have good news from Andrew! By the way, over the last days, outside trivial parsing issues, I noticed a rather annoying ICE on valid which should be rather easy to fix and has got **tons** of duplicates, just fyi (and Andrew's information): c++/67147, c++/87441, c++/80746, c++/79759, c++/86269 and more! Cheers, Paolo.
On 10/12/18 9:32 AM, Andrew Sutton wrote: >> >> BTW, I would discourage you from messing much with the concepts code >> at this point, as a major overhaul should be coming soon. > > Major overhaul: > > https://github.com/asutton/gcc (branch is concepts; we're about 2 weeks > back from trunk). Awesome! > Unfortunately, I we haven't been following GCC commit discipline, and > there's a bunch of dead/legacy code that I need to clean up. And of course > some regressions. I wanted to spend the weekend on this and forward a > cleaner version next week. But no time is as good as the present it seems. > > This fork reimplements concepts as currently specified in the WD (for the > most part). It also preserves TS syntax (but not behavior), although there > are certainly going to be some new bugs. > > -std=c++2a turns on concepts by default (sets -fconcepts) > -fconcepts-ts can be additionally specified to enable TS extensions > (abbreviated fn templates, etc). > -fconcepts on its own gives you (should give you) TS syntax with C++20 > semantics and no C++20 features. > > Here's what's changed: > - new requires clause syntax as required in the WD (-fconcepts-ts will > change this back to the TS syntax) > - concept bool is now a warning, although (IIRC) disabled with > -fconcepts-ts. Function and variable concepts live on. > - concepts are now their own kind of declaration (CONCEPT_DECL). That was a > big change. Can you say a bit about why that was better than continuing to use VAR_DECL? > - now only 3 kinds of constraints: conj, disj, and pred (should be > atomic, also needs a dead code cleanup). > - constraints on declarations are represented as expressions -- no > normalization until later > - associated constraints are only instantiated when checked -- no premature > substitution > - new implementaiton of satisfaction, does not require ahead-of-time > normalization > - new implementation of normalization (fewer nodes, smaller impl) > - atomic constraint comparison based on expr identity/parameter mapping > - complete rewrite of subsumption (new comparison model invalidated some > assumptions in the old impl) > - constrained decls differentiated by syntax of constraints (not > equivalence) > - moved the concepts testsuite into c++2a directory as a vetting/curating > process, new 2a tests, new ts-compatability tests > > There are some bugs and regressions. I know for a fact that we've broken > partial specialization of variable templats, but I'm really not sure how. > There's also probably a bug in the constraint comparison implementation > that affects partial ordering. More testing is needed. I mostly ignored the > TS support while updating to the WD semantics, so that's been a little > buggy when I brought it back online. Also, sometimes diagnostics aren't > emitted correctly. > > I'm not quite sure how to proceed with submitting this patch. Once I made > the decision to make concepts their own kind of declaration, the idea of > sending small patches went right out the window. Yeah, don't worry about trying to send small patches. I don't mind reviewing what's on the branch, though at least the final patch should be sent to the list for archival. What feedback are you looking for at this point? Jason
Sorry for the slow reply. I've been stuck working on some other projects. > Can you say a bit about why that was better than continuing to use > VAR_DECL? > I wanted to make sure that we avoid normal VAR_DECL processing routines, so we don't e.g., slip into a function where we might try to generate an address for a concept. Yeah, don't worry about trying to send small patches. I don't mind > reviewing what's on the branch, though at least the final patch should > be sent to the list for archival. > > What feedback are you looking for at this point? > Mostly anything that would obviously prevent or cause problems merging in the near future. I'll try to keep the asutton/gcc fork on github rebased on trunk so there shouldn't be too many merge issues.
On Wed, Oct 31, 2018 at 2:34 PM Andrew Sutton <andrew.n.sutton@gmail.com> wrote: > > Sorry for the slow reply. I've been stuck working on some other projects. >> >> Can you say a bit about why that was better than continuing to use VAR_DECL? > > I wanted to make sure that we avoid normal VAR_DECL processing routines, so we don't e.g., slip into a function where we might try to generate an address for a concept. > >> Yeah, don't worry about trying to send small patches. I don't mind >> reviewing what's on the branch, though at least the final patch should >> be sent to the list for archival. >> >> What feedback are you looking for at this point? > > Mostly anything that would obviously prevent or cause problems merging in the near future. > > I'll try to keep the asutton/gcc fork on github rebased on trunk so there shouldn't be too many merge issues. Any updates? Jason
Index: cp/parser.c =================================================================== --- cp/parser.c (revision 264805) +++ cp/parser.c (working copy) @@ -26110,10 +26114,11 @@ cp_parser_compound_requirement (cp_parser *parser) return finish_compound_requirement (expr, type, noexcept_p); } -/* Parse a nested requirement. This is the same as a requires clause. +/* Parse a nested requirement. This is the same as a requires clause followed + by a semicolon. nested-requirement: - requires-clause */ + requires-clause ';' */ static tree cp_parser_nested_requirement (cp_parser *parser) { @@ -26121,6 +26126,9 @@ cp_parser_nested_requirement (cp_parser *parser) tree req = cp_parser_requires_clause (parser); if (req == error_mark_node) return error_mark_node; + + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + return finish_nested_requirement (req); } Index: testsuite/g++.dg/concepts/pr67595.C =================================================================== --- testsuite/g++.dg/concepts/pr67595.C (revision 264805) +++ testsuite/g++.dg/concepts/pr67595.C (working copy) @@ -2,7 +2,7 @@ template <class X> concept bool allocatable = requires{{new X}->X * }; template <class X> concept bool semiregular = allocatable<X>; -template <class X> concept bool readable = requires{requires semiregular<X>}; +template <class X> concept bool readable = requires{requires semiregular<X>;}; template <class> int weak_input_iterator = requires{{0}->readable}; template <class X> bool input_iterator{weak_input_iterator<X>}; // { dg-warning "narrowing conversion" } template <class X> bool forward_iterator{input_iterator<X>}; Index: testsuite/g++.dg/concepts/pr71140.C =================================================================== --- testsuite/g++.dg/concepts/pr71140.C (nonexistent) +++ testsuite/g++.dg/concepts/pr71140.C (working copy) @@ -0,0 +1,8 @@ +// { dg-do compile { target c++14 } } +// { dg-additional-options "-fconcepts" } + +template<typename T> +concept bool C = requires(T t) { + requires true // { dg-error "expected .\;. before .\}. token" } +}; +static_assert(C<int>, "");