From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125694 invoked by alias); 16 Nov 2018 18:13:31 -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 125677 invoked by uid 89); 16 Nov 2018 18:13:31 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=Heres, Here's, Successfully, throwing X-HELO: mail-ot1-f65.google.com Received: from mail-ot1-f65.google.com (HELO mail-ot1-f65.google.com) (209.85.210.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Nov 2018 18:13:28 +0000 Received: by mail-ot1-f65.google.com with SMTP id n46so22136516otb.9 for ; Fri, 16 Nov 2018 10:13:27 -0800 (PST) MIME-Version: 1.0 References: <1542318534-62032-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1542318534-62032-1-git-send-email-dmalcolm@redhat.com> From: Jason Merrill Date: Fri, 16 Nov 2018 18:13:00 -0000 Message-ID: Subject: Re: [PATCH] v2: C/C++: add fix-it hints for missing '&' and '*' (PR c++/87850) To: David Malcolm Cc: Martin Sebor , Eric Gallager , gcc-patches List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg01522.txt.bz2 On Thu, Nov 15, 2018 at 4:48 PM David Malcolm wrote: > On Tue, 2018-11-13 at 16:34 -0500, Jason Merrill wrote: > > On Mon, Nov 12, 2018 at 4:32 PM Martin Sebor > > 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 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