From: Michel Morin <mimomorin@gmail.com>
To: Jason Merrill <jason@redhat.com>
Cc: David Malcolm <dmalcolm@redhat.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: fix wrong fixit hints for misspelled typedef [PR77565]
Date: Thu, 23 Sep 2021 08:05:34 +0900 [thread overview]
Message-ID: <CAO=jbbqqzx-T7O9bzvuhtL77cAyFUH+98zph4LpdK=PGaDyAPg@mail.gmail.com> (raw)
In-Reply-To: <0c128bcf-d67b-9ef3-94cf-3ced028cc51c@redhat.com>
On Thu, Sep 23, 2021 at 5:09 AM Jason Merrill <jason@redhat.com> wrote:
>
> On 9/21/21 20:53, Michel Morin wrote:
> > On Tue, Sep 21, 2021 at 5:24 AM Jason Merrill <jason@redhat.com> wrote:
> >>
> >> On 9/17/21 13:31, Michel Morin wrote:
> >>> On Fri, Sep 17, 2021 at 3:23 AM Jason Merrill <jason@redhat.com> wrote:
> >>>>
> >>>> On 9/16/21 11:50, Michel Morin wrote:
> >>>>> On Thu, Sep 16, 2021 at 5:44 AM Jason Merrill <jason@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 <dmalcolm@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
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
next prev parent reply other threads:[~2021-09-22 23:05 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-13 18:35 Michel Morin
2021-09-13 22:14 ` David Malcolm
2021-09-14 8:29 ` Michel Morin
2021-09-15 20:43 ` Jason Merrill
2021-09-16 15:50 ` Michel Morin
2021-09-16 18:23 ` Jason Merrill
2021-09-17 17:31 ` Michel Morin
2021-09-20 20:24 ` Jason Merrill
2021-09-22 0:53 ` Michel Morin
2021-09-22 20:08 ` Jason Merrill
2021-09-22 23:05 ` Michel Morin [this message]
2021-09-23 20:29 ` Jason Merrill
2021-09-24 0:49 ` Michel Morin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAO=jbbqqzx-T7O9bzvuhtL77cAyFUH+98zph4LpdK=PGaDyAPg@mail.gmail.com' \
--to=mimomorin@gmail.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).