On Thu, Sep 23, 2021 at 5:09 AM Jason Merrill <ja...@redhat.com> wrote:
>
> On 9/21/21 20:53, Michel Morin wrote:
> > On Tue, Sep 21, 2021 at 5:24 AM Jason Merrill <ja...@redhat.com> wrote:
> >>
> >> On 9/17/21 13:31, Michel Morin wrote:
> >>> On Fri, Sep 17, 2021 at 3:23 AM Jason Merrill <ja...@redhat.com> wrote:
> >>>>
> >>>> On 9/16/21 11:50, Michel Morin wrote:
> >>>>> On Thu, Sep 16, 2021 at 5:44 AM Jason Merrill <ja...@redhat.com> wrote:
> >>>>>>
> >>>>>> On 9/14/21 04:29, Michel Morin via Gcc-patches wrote:
> >>>>>>> On Tue, Sep 14, 2021 at 7:14 AM David Malcolm <dmalc...@redhat.com>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> On Tue, 2021-09-14 at 03:35 +0900, Michel Morin via Gcc-patches
> >>>>>>>> wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> PR77565 reports that, with the code `typdef int Int;`, GCC emits
> >>>>>>>>> "did you mean 'typeof'?" instead of "did you mean 'typedef'?".
> >>>>>>>>>
> >>>>>>>>> This happens because the typo corrector determines that `typeof` is
> >>>>>>>>> a
> >>>>>>>>> candidate for suggestion (through
> >>>>>>>>> `cp_keyword_starts_decl_specifier_p`),
> >>>>>>>>> but `typedef` is not.
> >>>>>>>>>
> >>>>>>>>> This patch fixes the issue by adding `typedef` as a candidate. The
> >>>>>>>>> patch
> >>>>>>>>> additionally adds the `inline` specifier and cv-specifiers as a
> >>>>>>>>> candidate.
> >>>>>>>>> Here is a patch (tests `make check-gcc` pass on darwin):
> >>>>>>>>
> >>>>>>>> Thanks for this patch (and for reporting the bug in the first place).
> >>>>>>>>
> >>>>>>>> I notice that, as well as being used for fix-it hints by
> >>>>>>>> lookup_name_fuzzy (indirectly via suggest_rid_p),
> >>>>>>>> cp_keyword_starts_decl_specifier_p is also used by
> >>>>>>>> cp_lexer_next_token_is_decl_specifier_keyword, which is used by
> >>>>>>>> cp_parser_lambda_declarator_opt and
> >>>>>>>> cp_parser_constructor_declarator_p.
> >>>>>>>
> >>>>>>> Ah, you're right! Thank you for pointing this out.
> >>>>>>> I failed to grep those functions somehow.
> >>>>>>>
> >>>>>>> One thing that confuses me is that cp_keyword_starts_decl_specifier_p
> >>>>>>> misses many keywords that can start decl-specifiers (e.g.
> >>>>>>> typedef/inline/cv-qual and friend/explicit/virtual).
> >>>>>>> So let's wait C++ frontend maintainers ;)
> >>>>>>
> >>>>>> That is strange. Let's add all the rest of them as well.
> >>>>>
> >>>>> Done. Thanks for your help!
> >>>>>
> >>>>> One more thing — cp_keyword_starts_decl_specifier_p includes
> >>>>> RID_ATTRIBUTE
> >>>>> (from the beginning; see https://gcc.gnu.org/PR28261 ), but attributes
> >>>>> are
> >>>>> not decl-specifiers. Would it be reasonable to remove this?
> >>>>
> >>>> It looks like the place that PR28261 used
> >>>> cp_lexer_next_token_is_decl_specifier_keyword specifically exempts
> >>>> attributes:
> >>>>
> >>>>> && (!cp_lexer_next_token_is_decl_specifier_keyword
> >>>>> (parser->lexer)
> >>>>> /* GNU attributes can actually appear both at the start
> >>>>> of
> >>>>> a parameter and parenthesized declarator.
> >>>>> S (__attribute__((unused)) int);
> >>>>> is a constructor, but
> >>>>> S (__attribute__((unused)) foo) (int);
> >>>>> is a function declaration. */
> >>>>> || (cp_parser_allow_gnu_extensions_p (parser)
> >>>>> && cp_next_tokens_can_be_gnu_attribute_p (parser)))
> >>>>
> >>>> So yes, let's remove RID_ATTRIBUTE and the || clause there. I'd keep
> >>>> the comment, but move it to go with the test for C++11 attributes below.
> >>>
> >>> Done. No regressions introduced.
> >>>
> >>>>> One more thing — cp_keyword_starts_decl_specifier_p includes
> >>>>> RID_ATTRIBUTE
> >>>>> (from the beginning; see https://gcc.gnu.org/PR28261 ), but attributes
> >>>>> are
> >>>>> not decl-specifiers.
> >>>
> >>> Oh, this is wrong. I thought that, since C++11 attributes are not a
> >>> decl-specifier, neither are GNU attributes. But the comment just before
> >>> cp_parser_decl_specifier_seq says that GNU attributes are considered as a
> >>> decl-specifier. So I'm not confident about the removal of RID_ATTRIBUTE in
> >>> cp_keyword_starts_decl_specifier_p...
> >>
> >> GNU attributes can appear in lots of places, and the only two callers of
> >> cp_parser_next_token_is_decl_specifier_keyword don't want to treat
> >> attributes accordingly.
> >
> > Makes sense.
> >
> >> Let's go with both your patches, and also
> >> remove the consequently-unnecessary attributes check in
> >> cp_parser_lambda_declarator_opt:
> >>
> >>> if (cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
> >>> && !cp_next_tokens_can_be_gnu_attribute_p (parser))
> >>
> >> OK with that change.
> >
> > Updated and rebased the patch. No regressions on x86_64-apple-darwin.
> >
> > Thank you for your help!
>
> Looks good, thanks. You can push the patches yourself, right?
This is my first patch contribution to GCC, and I don't have write access.
So it'd be great if someone pushes the patches.
I assume these patches are small enough that copyright assignment or
DCO certification are not needed. (If needed, I'll prepare one.)
> >>> I've split the patch into two. The first one is for adding missing
> >>> keywords to
> >>> fix PR77565 and the second one is for removing the "attribute" keyword.
> >>> Here is the second patch (if this is not applied, that's no problem ;) )
> >>>
> >>> ======================================================
> >>> c++: adjust the handling of RID_ATTRIBUTE.
> >>>
> >>> gcc/cp/ChangeLog:
> >>>
> >>> * parser.c (cp_keyword_starts_decl_specifier_p): Do not
> >>> handle RID_ATTRIBUTE.
> >>> (cp_parser_constructor_declarator_p): Remove now-redundant
> >>> checks.
> >>>
> >>> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> >>> index 40308d0d33f..d184a3aca7e 100644
> >>> --- a/gcc/cp/parser.c
> >>> +++ b/gcc/cp/parser.c
> >>> @@ -1062,7 +1062,6 @@ cp_keyword_starts_decl_specifier_p (enum rid
> >>> keyword)
> >>> case RID_TYPEDEF:
> >>> case RID_INLINE:
> >>> /* GNU extensions. */
> >>> - case RID_ATTRIBUTE:
> >>> case RID_TYPEOF:
> >>> /* C++11 extensions. */
> >>> case RID_DECLTYPE:
> >>> @@ -30798,23 +30797,22 @@ cp_parser_constructor_declarator_p
> >>> (cp_parser *parser, cp_parser_flags flags,
> >>> /* A parameter declaration begins with a decl-specifier,
> >>> which is either the "attribute" keyword, a storage class
> >>> specifier, or (usually) a type-specifier. */
> >>> - && (!cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
> >>> - /* GNU attributes can actually appear both at the start of
> >>> - a parameter and parenthesized declarator.
> >>> - S (__attribute__((unused)) int);
> >>> - is a constructor, but
> >>> - S (__attribute__((unused)) foo) (int);
> >>> - is a function declaration. */
> >>> - || (cp_parser_allow_gnu_extensions_p (parser)
> >>> - && cp_next_tokens_can_be_gnu_attribute_p (parser)))
> >>> - /* A parameter declaration can also begin with [[attribute]]. */
> >>> + && !cp_lexer_next_token_is_decl_specifier_keyword (parser->lexer)
> >>> + /* GNU attributes can actually appear both at the start of
> >>> + a parameter and parenthesized declarator.
> >>> + S (__attribute__((unused)) int);
> >>> + is a constructor, but
> >>> + S (__attribute__((unused)) foo) (int);
> >>> + is a function declaration. [[attribute]] can appear in the
> >>> + first form too, but not in the second form. */
> >>> && !cp_next_tokens_can_be_std_attribute_p (parser))
> >>> {
> >>> tree type;
> >>> tree pushed_scope = NULL_TREE;
> >>> unsigned saved_num_template_parameter_lists;
> >>>
> >>> - if (cp_next_tokens_can_be_gnu_attribute_p (parser))
> >>> + if (cp_parser_allow_gnu_extensions_p (parser)
> >>> + && cp_next_tokens_can_be_gnu_attribute_p (parser))
> >>> {
> >>> unsigned int n = cp_parser_skip_gnu_attributes_opt (parser, 1);
> >>> while (--n)
> >>> ======================================================
> >>>
> >>> Regards,
> >>> Michel
> >>>
> >>>
> >>>>> Both patches (with and without removal of RID_ATTRIBUTE) attached.
> >>>>> No regressions on x86_64-apple-darwin.
> >>>>>
> >>>>> Regards,
> >>>>> Michel
> >>>>>
> >>>>>
> >>>>>
> >>>>>>>> So I'm not sure if this fix is exactly correct - hopefully one of the
> >>>>>>>> C++ frontend maintainers can chime in. If
> >>>>>>>> cp_keyword_starts_decl_specifier_p isn't quite the right place for
> >>>>>>>> this, the fix could probably go in suggest_rid_p instead, which *is*
> >>>>>>>> specific to spelling corrections.
> >>>>>>>>
> >>>>>>>> Hope this is constructive; thanks again for the patch
> >>>>>>>> Dave
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> ============================================
> >>>>>>>>> c++: add typo corrections for typedef/inline/cv-qual [PR77565]
> >>>>>>>>>
> >>>>>>>>> PR c++/77565
> >>>>>>>>>
> >>>>>>>>> gcc/cp/ChangeLog:
> >>>>>>>>>
> >>>>>>>>> * parser.c (cp_keyword_starts_decl_specifier_p): Handle
> >>>>>>>>> typedef/inline specifiers and cv-qualifiers.
> >>>>>>>>>
> >>>>>>>>> gcc/testsuite/ChangeLog:
> >>>>>>>>>
> >>>>>>>>> * g++.dg/spellcheck-typenames.C: Add tests for decl-specs.
> >>>>>>>>>
> >>>>>>>>> --- a/gcc/cp/parser.c
> >>>>>>>>> +++ b/gcc/cp/parser.c
> >>>>>>>>> @@ -1051,6 +1051,12 @@ cp_keyword_starts_decl_specifier_p (enum rid
> >>>>>>>>> keyword)
> >>>>>>>>> case RID_FLOAT:
> >>>>>>>>> case RID_DOUBLE:
> >>>>>>>>> case RID_VOID:
> >>>>>>>>> + /* CV qualifiers. */
> >>>>>>>>> + case RID_CONST:
> >>>>>>>>> + case RID_VOLATILE:
> >>>>>>>>> + /* typedef/inline specifiers. */
> >>>>>>>>> + case RID_TYPEDEF:
> >>>>>>>>> + case RID_INLINE:
> >>>>>>>>> /* GNU extensions. */
> >>>>>>>>> case RID_ATTRIBUTE:
> >>>>>>>>> case RID_TYPEOF:
> >>>>>>>>> --- a/gcc/testsuite/g++.dg/spellcheck-typenames.C
> >>>>>>>>> +++ b/gcc/testsuite/g++.dg/spellcheck-typenames.C
> >>>>>>>>> @@ -76,3 +76,38 @@ singed char ch; // { dg-error "1: 'singed' does
> >>>>>>>>> not
> >>>>>>>>> name a type; did you mean 's
> >>>>>>>>> ^~~~~~
> >>>>>>>>> signed
> >>>>>>>>> { dg-end-multiline-output "" } */
> >>>>>>>>> +
> >>>>>>>>> +typdef int my_int; // { dg-error "1: 'typdef' does not name a type;
> >>>>>>>>> did you mean 'typedef'?" }
> >>>>>>>>> +/* { dg-begin-multiline-output "" }
> >>>>>>>>> + typdef int my_int;
> >>>>>>>>> + ^~~~~~
> >>>>>>>>> + typedef
> >>>>>>>>> + { dg-end-multiline-output "" } */
> >>>>>>>>> +
> >>>>>>>>> +inlien int inline_func(); // { dg-error "1: 'inlien' does not name
> >>>>>>>>> a
> >>>>>>>>> type; did you mean 'inline'?" }
> >>>>>>>>> +/* { dg-begin-multiline-output "" }
> >>>>>>>>> + inlien int inline_func();
> >>>>>>>>> + ^~~~~~
> >>>>>>>>> + inline
> >>>>>>>>> + { dg-end-multiline-output "" } */
> >>>>>>>>> +
> >>>>>>>>> +coonst int ci = 0; // { dg-error "1: 'coonst' does not name a type;
> >>>>>>>>> did you mean 'const'?" }
> >>>>>>>>> +/* { dg-begin-multiline-output "" }
> >>>>>>>>> + coonst int ci = 0;
> >>>>>>>>> + ^~~~~~
> >>>>>>>>> + const
> >>>>>>>>> + { dg-end-multiline-output "" } */
> >>>>>>>>> +
> >>>>>>>>> +voltil int vi; // { dg-error "1: 'voltil' does not name a type; did
> >>>>>>>>> you mean 'volatile'?" }
> >>>>>>>>> +/* { dg-begin-multiline-output "" }
> >>>>>>>>> + voltil int vi;
> >>>>>>>>> + ^~~~~~
> >>>>>>>>> + volatile
> >>>>>>>>> + { dg-end-multiline-output "" } */
> >>>>>>>>> +
> >>>>>>>>> +statik int si; // { dg-error "1: 'statik' does not name a type; did
> >>>>>>>>> you mean 'static'?" }
> >>>>>>>>> +/* { dg-begin-multiline-output "" }
> >>>>>>>>> + statik int si;
> >>>>>>>>> + ^~~~~~
> >>>>>>>>> + static
> >>>>>>>>> + { dg-end-multiline-output "" } */
> >>>>>>>>> ============================================
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Regards,
> >>>>>>>>> Michel
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
