From: Jason Merrill <jason@redhat.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: Martin Sebor <msebor@gmail.com>,
Eric Gallager <egall@gwmail.gwu.edu>,
gcc-patches List <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] v2: C/C++: add fix-it hints for missing '&' and '*' (PR c++/87850)
Date: Fri, 16 Nov 2018 18:13:00 -0000 [thread overview]
Message-ID: <CADzB+2nMUVXThn=8_CeurHmCo2Rckqy+37RuEYBSFNqsCkY20Q@mail.gmail.com> (raw)
In-Reply-To: <1542318534-62032-1-git-send-email-dmalcolm@redhat.com>
On Thu, Nov 15, 2018 at 4:48 PM David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2018-11-13 at 16:34 -0500, Jason Merrill wrote:
> > On Mon, Nov 12, 2018 at 4:32 PM Martin Sebor <msebor@gmail.com>
> > wrote:
> > > On 11/11/2018 02:02 PM, David Malcolm wrote:
> > > > On Sun, 2018-11-11 at 11:01 -0700, Martin Sebor wrote:
> > > > > On 11/10/2018 12:01 AM, Eric Gallager wrote:
> > > > > > On 11/9/18, David Malcolm <dmalcolm@redhat.com> wrote:
> > > > > > > This patch adds a fix-it hint to various pointer-vs-non-
> > > > > > > pointer
> > > > > > > diagnostics, suggesting the addition of a leading '&' or
> > > > > > > '*'.
> > > > > > >
> > > > > > > For example, note the ampersand fix-it hint in the
> > > > > > > following:
> > > > > > >
> > > > > > > demo.c:5:22: error: invalid conversion from 'pthread_key_t'
> > > > > > > {aka
> > > > > > > 'unsigned
> > > > > > > int'}
> > > > > > > to 'pthread_key_t*' {aka 'unsigned int*'} [-fpermissive]
> > > > > > > 5 | pthread_key_create(key, NULL);
> > > > > > > | ^~~
> > > > > > > | |
> > > > > > > | pthread_key_t {aka unsigned
> > > > > > > int}
> > > > > > > | &
> > > > > >
> > > > > > Having both the type and the fixit underneath the caret looks
> > > > > > kind
> > > > > > of confusing
> > > > >
> > > > > I agree it's rather subtle. Keeping the diagnostics separate
> > > > > from
> > > > > the suggested fix should avoid the confusion.
> > > >
> > > > FWIW, the fix-it hint is in a different color (assuming that gcc
> > > > is
> > > > invoked in an environment that prints that...)
> > >
> > > I figured it would be, but I'm still not sure it's good design
> > > to be relying on color alone to distinguish between the problem
> > > and the suggested fix. Especially when they are so close to one
> > > another and the fix is just a single character with no obvious
> > > relationship to the rest of the text on the screen. In other
> > > warnings there's at least the "did you forget the '@'?" part
> > > to give a clue, even though even there the connection between
> > > the "did you forget" and the & several lines down wouldn't
> > > necessarily be immediately apparent.
> >
> > Agreed, something along those lines would help to understand why the
> > compiler is throwing a random & into the diagnostic.
> >
> > Jason
>
> Here's an updated version which adds a note, putting the fix-it hint
> on that instead (I attempted adding the text to the initial error,
> but there was something of a combinatorial explosion of messages).
>
> The above example becomes:
>
> demo.c: In function 'int main()':
> demo.c:5:22: error: invalid conversion from 'pthread_key_t' {aka 'unsigned int'}
> to 'pthread_key_t*' {aka 'unsigned int*'} [-fpermissive]
> 5 | pthread_key_create(key, NULL);
> | ^~~
> | |
> | pthread_key_t {aka unsigned int}
> demo.c:5:22: note: possible fix: take the address with '&'
> 5 | pthread_key_create(key, NULL);
> | ^~~
> | &
> In file included from demo.c:1:
> /usr/include/pthread.h:1122:47: note: initializing argument 1 of
> 'int pthread_key_create(pthread_key_t*, void (*)(void*))'
> 1122 | extern int pthread_key_create (pthread_key_t *__key,
> | ~~~~~~~~~~~~~~~^~~~~
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> gcc/c-family/ChangeLog:
> PR c++/87850
> * c-common.c: Include "gcc-rich-location.h".
> (maybe_emit_indirection_note): New function.
> * c-common.h (maybe_emit_indirection_note): New decl.
>
> gcc/c/ChangeLog:
> PR c++/87850
> * c-typeck.c (convert_for_assignment): Call
> maybe_emit_indirection_note for pointer vs non-pointer
> diagnostics.
>
> gcc/cp/ChangeLog:
> PR c++/87850
> * call.c (convert_like_real): Call
> maybe_emit_indirection_note for "invalid conversion" diagnostic.
>
> gcc/testsuite/ChangeLog:
> PR c++/87850
> * c-c++-common/indirection-fixits.c: New test.
> ---
> gcc/c-family/c-common.c | 33 +++
> gcc/c-family/c-common.h | 2 +
> gcc/c/c-typeck.c | 10 +-
> gcc/cp/call.c | 2 +
> gcc/testsuite/c-c++-common/indirection-fixits.c | 270 ++++++++++++++++++++++++
> 5 files changed, 315 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/indirection-fixits.c
>
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index cd88f3a..d5c7c7f 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. If not see
> #include "gimplify.h"
> #include "substring-locations.h"
> #include "spellcheck.h"
> +#include "gcc-rich-location.h"
> #include "selftest.h"
>
> cpp_reader *parse_in; /* Declared in c-pragma.h. */
> @@ -8405,6 +8406,38 @@ maybe_suggest_missing_token_insertion (rich_location *richloc,
> }
> }
>
> +/* Potentially emit a note about likely missing '&' or '*',
> + depending on EXPR and EXPECTED_TYPE. */
> +
> +void
> +maybe_emit_indirection_note (location_t loc,
> + tree expr, tree expected_type)
> +{
> + gcc_assert (expr);
> + gcc_assert (expected_type);
> +
> + tree actual_type = TREE_TYPE (expr);
> +
> + /* Missing '&'. */
> + if (TREE_CODE (expected_type) == POINTER_TYPE
> + && actual_type == TREE_TYPE (expected_type)
Type comparison should use same_type_p, not ==, so that we deal
properly with typedef variants. OK with that change.
Jason
next prev parent reply other threads:[~2018-11-16 18:13 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-09 22:36 [PATCH] " David Malcolm
2018-11-10 7:01 ` Eric Gallager
2018-11-11 18:01 ` Martin Sebor
2018-11-11 21:02 ` David Malcolm
2018-11-12 21:32 ` Martin Sebor
2018-11-13 21:34 ` Jason Merrill
2018-11-15 21:48 ` [PATCH] v2: " David Malcolm
2018-11-16 18:13 ` Jason Merrill [this message]
2018-11-19 21:23 ` [PATCH] v3: " David Malcolm
2018-11-20 2:46 ` Joseph Myers
2018-11-20 22:05 ` David Malcolm
2018-11-20 22:23 ` Joseph Myers
2018-11-30 23:01 ` [PATCH] v4: " David Malcolm
2018-12-01 18:38 ` Jason Merrill
2018-12-03 22:14 ` Joseph Myers
2018-12-05 16:03 ` Jason Merrill
2018-12-05 16:18 ` David Malcolm
2018-11-21 0:36 ` [PATCH] v3: " Jeff Law
2018-11-21 0:39 ` Martin Sebor
2018-11-30 15:27 ` David Malcolm
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='CADzB+2nMUVXThn=8_CeurHmCo2Rckqy+37RuEYBSFNqsCkY20Q@mail.gmail.com' \
--to=jason@redhat.com \
--cc=dmalcolm@redhat.com \
--cc=egall@gwmail.gwu.edu \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@gmail.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).