Bug 24985 (caret) - caret diagnostics
Summary: caret diagnostics
Status: RESOLVED FIXED
Alias: caret
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 3.4.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/wiki/Better_Diagno...
Keywords: diagnostic
: 40228 52655 (view as bug list)
Depends on:
Blocks: 49152
  Show dependency treegraph
 
Reported: 2005-11-22 04:01 UTC by Ivan Godard
Modified: 2018-08-10 02:22 UTC (History)
8 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-10-07 21:25:54


Attachments
caret diagnostics patch (2.65 KB, patch)
2012-04-04 15:28 UTC, Manuel López-Ibáñez
Details | Diff
Patch to add -fno-diagnostics-show-caret for testing (303 bytes, patch)
2012-04-04 18:47 UTC, Jason Merrill
Details | Diff
Patch to prune caret diagnostics from gcc output (458 bytes, patch)
2012-04-04 19:19 UTC, Jason Merrill
Details | Diff
fix overload carets (2.69 KB, patch)
2012-04-13 22:59 UTC, Manuel López-Ibáñez
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Godard 2005-11-22 04:01:36 UTC
Showing the source line in the diagnostic, with a caret ("^") inder the column in error, is much friendlier. The best I've ever seen also was graceful when the error was inside a maco expansion; it showed the position in the token list of each nested macro, limited to a dozen or so tokens on either side of the error (or nested macro invoke) in case of long macro bodies. Macros are less common in C++, but are still used heavily in legacy code (and by legacy programmers :-) ). Figuring out the expansion by hand, or having to do a -E and then compile the expanded source, are both pains in the butt.

Ivan
Comment 1 Andrew Pinski 2005-11-22 05:11:27 UTC
Confirmed, this was already a known issue.  There is another bug mentioning the caret.
Comment 2 Manuel López-Ibáñez 2008-02-27 09:00:57 UTC
This just needs someone with the time and willingness to implement it.
Comment 3 Manuel López-Ibáñez 2008-07-22 09:50:09 UTC
Better summary to find duplicates.
Comment 4 Manuel López-Ibáñez 2009-07-02 23:49:50 UTC
*** Bug 40228 has been marked as a duplicate of this bug. ***
Comment 5 Paolo Carlini 2009-07-02 23:55:08 UTC
Manuel, pardon the naive question: are we getting closer to fixing this? I'm asking because I saw patches about column numbers and wondered if that really means that very soon we'll be able to just print a caret instead ;)
Comment 6 Manuel López-Ibáñez 2009-07-03 00:25:57 UTC
(In reply to comment #5)
> Manuel, pardon the naive question: are we getting closer to fixing this? I'm
> asking because I saw patches about column numbers and wondered if that really
> means that very soon we'll be able to just print a caret instead ;)

Not really, if we ever have caret diagnostics, then it will be more accurate, but it does not get us closer to print a source line. Of course, anything is better than nothing and accurate column numbers are probably useful even without caret diagnostics.

The fundamental thing missing is a location_t-> const char * (source line) translator. Quoting from http://gcc.gnu.org/wiki/Better_Diagnostics

3) A location(s) -> source strings interface and machinery. Ideally,
     this should be more or less independent of CPP, so CPP (through
     the diagnostics machinery) calls into this when needed and not
     the other way around. This can be implemented in several ways:

     a) Keeping the CPP buffers in memory and having in line-maps
        pointers directly into the buffers contents. This is easy and
        fast but potentially memory consuming. Care to handle
        charsets, tabs, etc must be taken into account. Factoring out
        anything useful from libcpp would help to implement this.

     b) Re-open the file and fseek. This is not trivial since we need
        to do it fast but still do all character conversions that we
        did when libcpp opened it the first time. This is
        approximately what Clang (LLVM) does and it seems they can do
        it very fast by keeping a cache of buffers ever reopened. I
        think that thanks to our line-maps implementation, we can do
        the seeking quite more efficiently in terms of computation
        time.  However, opening files is quite embedded into CPP, so
        that would need to be factored out so we can avoid any
        unnecessary CPP stuff when reopening but still do it
        *properly* and *efficiently*.

Comment 7 Paolo Carlini 2009-07-03 00:38:42 UTC
I see, thanks Manuel for the feedback, I'll give it some thought.
Comment 8 Dr. David Alan Gilbert 2009-07-03 11:03:54 UTC
Note there are two slightly different things being asked for here:
  1) Showing the horizontal position in the line
  2) show the preprocessed line rather than the raw line (which was my 40228 that just got marked as a dupe) - obviously this would be an option rather than the default.

I'm not entirely sure they are actually dupes.

Dave

Comment 9 Benjamin Kosnik 2009-10-07 16:56:53 UTC
Beyond caret diagnostics there is also range info for the diagnostic location.

See:
http://clang.llvm.org/diagnostics.html

Comment 10 Manuel López-Ibáñez 2009-10-07 17:06:57 UTC
(In reply to comment #9)
> Beyond caret diagnostics there is also range info for the diagnostic location.

See the patch and thread at

http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00459.html

and the requested patch:

http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00459.html

Feel free to take anything and do as you please.
Comment 11 Jason Merrill 2009-10-07 18:31:48 UTC
(In reply to comment #10)
> http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00459.html

That's a vasprintf patch, I don't see the connection.
Comment 12 Manuel López-Ibáñez 2009-10-07 21:25:53 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00459.html
> 
> That's a vasprintf patch, I don't see the connection.
> 

Ops, bad first link. This is the correct:

http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00174.html

Comment 13 Benjamin Kosnik 2009-10-15 17:33:47 UTC
Patch for change to coding conventions:
http://gcc.gnu.org/ml/gcc-patches/2009-10/msg00687.html

Comment 14 Manuel López-Ibáñez 2009-10-15 18:51:14 UTC
If you have half-baked patches, mention them in a comment or attach them to this bug. 

If you have complete patches, submit them to gcc-patches and link to them from here.

If you have ideas about how this could be implemented, where in the source code, or functions and data structures that could be helpful, please add them to the wiki page: http://gcc.gnu.org/wiki/Better_Diagnostics 
Comment 15 Manuel López-Ibáñez 2012-03-21 12:05:08 UTC
*** Bug 52655 has been marked as a duplicate of this bug. ***
Comment 16 Manuel López-Ibáñez 2012-04-04 15:26:50 UTC
@Jason,

This is my current patch. It directly opens the file a looks for the line. 

I see several improvements that could be made, like handling very long lines in a smarter way, a cache of already opened files, and moving all this code to libcpp (or even better to a separate liblinemap library). But there are improvements that could be done post-commit.

This patch bootstraps for languages=all,ada with both the default set to off and on, and regression tested with the default set to off, because I couldn't find a good way to tell DejaGNU to always use -fno-diagnostics-show-caret. Does anyone know the correct way to do this? This is the only missing piece.

If it looks ok to you, I will write a changelog and submit it to gcc-patches.
Comment 17 Manuel López-Ibáñez 2012-04-04 15:28:08 UTC
Created attachment 27089 [details]
caret diagnostics patch
Comment 18 Paolo Carlini 2012-04-04 15:49:33 UTC
Oh my, Manuel, yesterday I was walking by myself and it occurred to me that I didn't understand at all why we are not doing *already* what you are suggesting now!! Of course the idea came from Jason's comment about using a script. I said to myself: if we can do it so easily with a script, why can't we do it as part of GCC itself?!? (I'm not touching at all the issues with existing bugs in the column information, ranges, etc)
Comment 19 Jason Merrill 2012-04-04 18:47:58 UTC
Created attachment 27092 [details]
Patch to add -fno-diagnostics-show-caret for testing

This seems like what you want for the last bit.
Comment 20 Manuel López-Ibáñez 2012-04-04 18:52:32 UTC
(In reply to comment #19)
> Created attachment 27092 [details]
> Patch to add -fno-diagnostics-show-caret for testing
> 
> This seems like what you want for the last bit.

Great, this works for C++, however, the caret is enabled for diagnostic that uses the diagnostics machinery, independently of the language (for example, warnings from the middle-end). If there is no general way, I guess I can do this for every language.
Comment 21 Jason Merrill 2012-04-04 19:19:21 UTC
Created attachment 27093 [details]
Patch to prune caret diagnostics from gcc output

Actually, this seems like a better approach: rather than disable the caret diagnostics, prune them from the output.  This regexp will remove any pair of lines where the first starts with a space and the second consists of spaces followed by a caret.
Comment 22 Manuel López-Ibáñez 2012-04-05 12:13:01 UTC
(In reply to comment #21)
> Created attachment 27093 [details]
> Patch to prune caret diagnostics from gcc output
> 
> Actually, this seems like a better approach: rather than disable the caret
> diagnostics, prune them from the output.  This regexp will remove any pair of
> lines where the first starts with a space and the second consists of spaces
> followed by a caret.

Unfortunately this does not work for several reasons:

* DejaGNU trims leading whitespace before passing the text to prune.
* DejaGNU matches as many lines as possible for each dg-* directive, so if you have something in the source line printed that is matched by a dg-* directive, the line is removed and the above regexp fails to prune the caret line.

The problem is that the default regexp of DejaGNU is:

"(^|\n)(\[^\n\]+$line\[^\n\]*($pattern)\[^\n\]*\n?)+"

the "+" at the end means that it matches as many lines as possible. It also matches any trailing and any leading whitespace. That is, it is impossible to tell to DejaGNU to match some line exactly.
Comment 23 Jason Merrill 2012-04-05 17:00:59 UTC
(In reply to comment #22)
> * DejaGNU trims leading whitespace before passing the text to prune.

So it does.  Bizarre.

            set comp_output [string trimleft $comp_output]

I guess we can just remove the initial space from that prune pattern.

> The problem is that the default regexp of DejaGNU is:
> 
> "(^|\n)(\[^\n\]+$line\[^\n\]*($pattern)\[^\n\]*\n?)+"
> 
> the "+" at the end means that it matches as many lines as possible.

Yes, but only lines that match the line number followed by the pattern; the source line shouldn't have its own line number before the pattern.

However, if an expected error contains .*, it will typically match both the error message and the source line, so any .* in testcases would need to change to \[^\n\].
Comment 24 Manuel López-Ibáñez 2012-04-05 22:05:15 UTC
(In reply to comment #23)
> (In reply to comment #22)
> > * DejaGNU trims leading whitespace before passing the text to prune.
> 
> So it does.  Bizarre.
> 
>             set comp_output [string trimleft $comp_output]
> 
> I guess we can just remove the initial space from that prune pattern.
> 
> > The problem is that the default regexp of DejaGNU is:
> > 
> > "(^|\n)(\[^\n\]+$line\[^\n\]*($pattern)\[^\n\]*\n?)+"
> > 
> > the "+" at the end means that it matches as many lines as possible.
> 
> Yes, but only lines that match the line number followed by the pattern; the
> source line shouldn't have its own line number before the pattern.

There are a few dg- directives with line number 0. They match everywhere.

Another problem with c99-vla-jump-3.c and similar testcases is that it seems as if DejaGNU limits the output (or has a limited size buffer for text) and decides to stop parsing in the middle of a source line, which breaks the pruning (and misses several diagnostics that need to be matched). Could this be true or am I doing something wrong?

I am starting to reconsider adding -fno-diagnostics-show-caret everywhere.
Comment 25 Manuel López-Ibáñez 2012-04-05 22:09:30 UTC
> Another problem with c99-vla-jump-3.c and similar testcases is that it seems as
> if DejaGNU limits the output (or has a limited size buffer for text) and
> decides to stop parsing in the middle of a source line, which breaks the
> pruning (and misses several diagnostics that need to be matched). Could this be
> true or am I doing something wrong?

PR12096 ?
Comment 26 Manuel López-Ibáñez 2012-04-07 12:53:34 UTC
A sneak-peek into the future ;-)

manuel@gcc12:~/test2/186200M/build/gcc$ ./cc1 -ftrack-macro-expansion ~/macro-clang.c 
 foo
/home/manuel/macro-clang.c: In function ‘foo’:
/home/manuel/macro-clang.c:2:94: error: invalid operands to binary < (have ‘struct mystruct’ and ‘float’)
 #define MYMAX(A,B)    __extension__ ({ __typeof__(A) __a = (A); __typeof__(B) __b = (B); __a < __b ? __b : __a; })
                                                                                              ^
/home/manuel/macro-clang.c:2:94: note: in expansion of macro 'MYMAX'
 #define MYMAX(A,B)    __extension__ ({ __typeof__(A) __a = (A); __typeof__(B) __b = (B); __a < __b ? __b : __a; })
                                                                                              ^
/home/manuel/macro-clang.c:8:11: note: expanded from here
 float x = MYMAX(p, f);
           ^

This is not possible with the patch just posted, but I will send this follow up after the initial part is committed. Of course, it needs some fine-tuning to avoid repetitions, but this is a problem with the locations given by the macro expansion unwinder, which, in my opinion, doesn't give the best location for each message and already produces repetitions. See:

/home/manuel/macro-caret.c: In function ‘g’:
/home/manuel/macro-caret.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
   OPERATE (A,<<,B)
              ^
/home/manuel/macro-caret.c:2:9: note: in expansion of macro 'OPERATE'
   OPRD1 OPRT OPRD2;
         ^
/home/manuel/macro-caret.c:5:3: note: expanded from here
   OPERATE (A,<<,B)
   ^
/home/manuel/macro-caret.c:5:14: note: in expansion of macro 'SHIFTL'
   OPERATE (A,<<,B)
              ^
/home/manuel/macro-caret.c:8:3: note: expanded from here
   SHIFTL (A,1)
   ^
/home/manuel/macro-caret.c:8:3: note: in expansion of macro 'MULT'
   SHIFTL (A,1)
   ^
/home/manuel/macro-caret.c:13:3: note: expanded from here
   MULT (1.0);// 1.0 << 1; <-- so this is an error.
   ^


Another possibility could be to print the preprocessed string when saying "in expansion". However, I am not sure how hard it will be to obtain the preprocessed string (or to re-create it at runtime). Help and pointers on how to achieve that are welcome.

Dodji, are you planning to propose to enable -ftrack-macro-expansion by default in GCC 4.8?
Comment 27 dodji@seketeli.org 2012-04-09 16:04:46 UTC
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> a écrit:

> Dodji, are you planning to propose to enable -ftrack-macro-expansion
> by default in GCC 4.8?

Wow, what a timing!

I am working on a patch-set to enable -ftrack-macro-expansion by default
for GCC 4.8.  It appeared that quite some fixes were still needed for
that ;-) It seems to pass bootstrap for C/C++ at the moment, so I think
I'll start polishing & flushing it out to gcc-patches shortly.

For the curious, my current tree is browsable at
http://seketeli.net/git/~dodji/gcc.git/log/?h=track-locs-by-default

To get the code, do:

    git clone git://seketeli.net/~dodji/gcc gcc.git
    cd gcc.git
    git checkout track-locs-by-default
Comment 28 dodji@seketeli.org 2012-04-09 16:19:15 UTC
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> a écrit:

> Of course, it needs some fine-tuning to avoid repetitions, but this is
> a problem with the locations given by the macro expansion unwinder,
> which, in my opinion, doesn't give the best location for each message
> and already produces repetitions. See:
>
> /home/manuel/macro-caret.c: In function ‘g’:
> /home/manuel/macro-caret.c:5:14: error: invalid operands to binary << (have
> ‘double’ and ‘int’)
>    OPERATE (A,<<,B)
>               ^
> /home/manuel/macro-caret.c:2:9: note: in expansion of macro 'OPERATE'
>    OPRD1 OPRT OPRD2;
>          ^
> /home/manuel/macro-caret.c:5:3: note: expanded from here
>    OPERATE (A,<<,B)
>    ^
> /home/manuel/macro-caret.c:5:14: note: in expansion of macro 'SHIFTL'
>    OPERATE (A,<<,B)
>               ^
> /home/manuel/macro-caret.c:8:3: note: expanded from here
>    SHIFTL (A,1)
>    ^
> /home/manuel/macro-caret.c:8:3: note: in expansion of macro 'MULT'
>    SHIFTL (A,1)
>    ^
> /home/manuel/macro-caret.c:13:3: note: expanded from here
>    MULT (1.0);// 1.0 << 1; <-- so this is an error.
>    ^

Could you please file a specific bug for this and CC me?  I'd be
interested to give this a look.

Thanks.
Comment 29 Manuel López-Ibáñez 2012-04-11 09:26:55 UTC
Author: manu
Date: Wed Apr 11 09:26:48 2012
New Revision: 186305

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186305
Log:
2012-04-11  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 24985
gcc/
        * diagnostic.h (show_caret): Declare.
	(caret_max_width): Declare.
	(diagnostic_show_locus): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
	(getenv_columns): New.
	(adjust_line): New.
	(diagnostic_set_caret_max_width): New.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
	* pretty-print.h (pp_get_prefix): New.
	(pp_base_get_prefix): New.
        * common.opt (fdiagnostics-show-caret): New option.
	* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
        * lib/prune.exp: Add -fno-diagnostics-show-caret.
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/common.opt
    trunk/gcc/diagnostic.c
    trunk/gcc/diagnostic.h
    trunk/gcc/doc/invoke.texi
    trunk/gcc/dwarf2out.c
    trunk/gcc/input.c
    trunk/gcc/input.h
    trunk/gcc/opts.c
    trunk/gcc/pretty-print.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/lib/prune.exp
    trunk/gcc/toplev.c
    trunk/libmudflap/ChangeLog
    trunk/libmudflap/testsuite/lib/libmudflap.exp
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/testsuite/lib/prune.exp
Comment 30 Manuel López-Ibáñez 2012-04-11 09:52:26 UTC
So we have caret diagnostics in GCC 4.8! :-) :-) :-)

In any case, I am leaving this open. There are some improvements that I still would like to make before 4.8.0, and surely there will be some bugs found. 

If you have specific improvements/bugs that you will love to see implemented/fixed, please add them as blockers of this PR.

A few things I am NOT going to work on, but it would be great if someone implemented them:

* Caching of file buffers: Since we often reopen+seek on the same file, a cache of recently opened files would (probably) speed up things. However, I guess that the OS has such cache, because at least in GNU/Linux, it feels fast enough. So, I am not 100% sure how much this will save versus how much overhead it will add.

* Handling wide chars, tabs, etc, according to GNU Coding Standards. There are some bugs in libcpp about this, so those would need to be fixed first.

* Fixing DejaGNU so we can parse the caret lines conveniently. Fixing this will also fix other long-standing testsuite bugs.

* Start/end location pairs for expressions so we can print a single expression (or a range) instead of the whole source line. See A.1 and A.4 in http://gcc.gnu.org/wiki/Better_Diagnostics


@Paolo,

I don't see any reason why your patch replacing %qE with %qT cannot go in now.

@Dodji

If -ftrack-macro-expansion becomes the default in 4.8, this will be an awesome release!
Comment 31 Jason Merrill 2012-04-12 21:44:23 UTC
The effect of this patch on overload resolution diagnostics is problematic:

-----
void f();
void f(int,int);

int main()
{
  f(1);
}
-----

wa2.C: In function ‘int main()’:
wa2.C:6:6: error: no matching function for call to ‘f(int)’
   f(1);
      ^
wa2.C:6:6: note: candidates are:
   f(1);
      ^
wa2.C:1:6: note: void f()
 void f();
      ^
wa2.C:1:6: note:   candidate expects 0 arguments, 1 provided
 void f();
      ^
wa2.C:2:6: note: void f(int, int)
 void f(int,int);
      ^
wa2.C:2:6: note:   candidate expects 2 arguments, 1 provided
 void f(int,int);
      ^

When there are multiple diagnostics at the same input location, we should only print the source/caret information once.
Comment 32 Manuel López-Ibáñez 2012-04-12 21:58:59 UTC
(In reply to comment #31)
> The effect of this patch on overload resolution diagnostics is problematic:
> wa2.C: In function ‘int main()’:
> wa2.C:6:6: error: no matching function for call to ‘f(int)’
>    f(1);
>       ^
> wa2.C:6:6: note: candidates are:
>    f(1);
>       ^
> wa2.C:1:6: note: void f()
>  void f();
>       ^
> wa2.C:1:6: note:   candidate expects 0 arguments, 1 provided
>  void f();
>       ^
> wa2.C:2:6: note: void f(int, int)
>  void f(int,int);
>       ^
> wa2.C:2:6: note:   candidate expects 2 arguments, 1 provided
>  void f(int,int);
>       ^
> 
> When there are multiple diagnostics at the same input location, we should only
> print the source/caret information once.

True. Actually, in this case, perhaps we should print:

 wa2.C:6:6: error: no matching function for call to ‘f(int)’
    f(1);
       ^
            note: candidates are:
 wa2.C:1:6: note:   candidate expects 0 arguments, 1 provided
  void f();
       ^
 wa2.C:2:6: note:   candidate expects 2 arguments, 1 provided
  void f(int,int);
       ^

no? Any other suggestions?

We could also print the %qD in the same line as:

 wa2.C:6:6: error: no matching function for call to ‘f(int)’
    f(1);
       ^
            note: candidates are:
 wa2.C:1:6: note:   candidate 'void f()' expects 0 arguments, 1 provided
  void f();
       ^
 wa2.C:2:6: note:   candidate 'void f(int, int)' expects 2 arguments, 1 provided
  void f(int,int);
       ^

What do you think?
Comment 33 Paolo Carlini 2012-04-13 02:25:48 UTC
Thanks Manu for the reminder, I have a couple of pending things in my TODO and then I will resurrect it for the great 'caret times' ;)
Comment 34 Richard Biener 2012-04-13 09:56:48 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > The effect of this patch on overload resolution diagnostics is problematic:
> > wa2.C: In function ‘int main()’:
> > wa2.C:6:6: error: no matching function for call to ‘f(int)’
> >    f(1);
> >       ^
> > wa2.C:6:6: note: candidates are:
> >    f(1);
> >       ^
> > wa2.C:1:6: note: void f()
> >  void f();
> >       ^
> > wa2.C:1:6: note:   candidate expects 0 arguments, 1 provided
> >  void f();
> >       ^
> > wa2.C:2:6: note: void f(int, int)
> >  void f(int,int);
> >       ^
> > wa2.C:2:6: note:   candidate expects 2 arguments, 1 provided
> >  void f(int,int);
> >       ^
> > 
> > When there are multiple diagnostics at the same input location, we should only
> > print the source/caret information once.
> 
> True. Actually, in this case, perhaps we should print:
> 
>  wa2.C:6:6: error: no matching function for call to ‘f(int)’
>     f(1);
>        ^
>             note: candidates are:
>  wa2.C:1:6: note:   candidate expects 0 arguments, 1 provided
>   void f();
>        ^
>  wa2.C:2:6: note:   candidate expects 2 arguments, 1 provided
>   void f(int,int);
>        ^
> 
> no? Any other suggestions?
> 
> We could also print the %qD in the same line as:
> 
>  wa2.C:6:6: error: no matching function for call to ‘f(int)’
>     f(1);
>        ^
>             note: candidates are:
>  wa2.C:1:6: note:   candidate 'void f()' expects 0 arguments, 1 provided
>   void f();
>        ^
>  wa2.C:2:6: note:   candidate 'void f(int, int)' expects 2 arguments, 1
> provided
>   void f(int,int);
>        ^
> 
> What do you think?

I think we should simply omit carets for all 'note's for now and add a
way for the callers to suppress carets.  Though if you consider the testcase
changed to

void f(); void f(int,int);

int main()
{
  f(1);
}

then suddenly the carets get more useful and the situation less clear:

t.C: In function 'int main()':
t.C:5:6: error: no matching function for call to 'f(int)'
   f(1);
      ^
t.C:5:6: note: candidates are:
   f(1);
      ^
t.C:1:6: note: void f()
 void f();  void f(int,int);
      ^
t.C:1:6: note:   candidate expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^
t.C:1:17: note: void f(int, int)
 void f();  void f(int,int);
                 ^
t.C:1:17: note:   candidate expects 2 arguments, 1 provided
 void f();  void f(int,int);
                 ^

btw, why do we print a location info for

t.C:5:6: note: candidates are:
   f(1);
      ^

at all?

t.C:1:6: note:   candidate expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^
t.C:1:17: note: void f(int, int)
 void f();  void f(int,int);
                 ^

and the 2nd note here looks wrong.
Comment 35 Manuel López-Ibáñez 2012-04-13 10:59:17 UTC
(In reply to comment #34)
> 
> btw, why do we print a location info for
> 
> t.C:5:6: note: candidates are:
>    f(1);
>       ^
> 
> at all?

I am proposing to print just: "note: candidates are:", with enough leading empty space to align with the previous error and no caret. Is that OK? Richard, Jason?

> 
> t.C:1:6: note:   candidate expects 0 arguments, 1 provided
>  void f();  void f(int,int);
>       ^
> t.C:1:17: note: void f(int, int)
>  void f();  void f(int,int);
>                  ^
> 
> and the 2nd note here looks wrong.

Could you explain why?
Comment 36 Richard Biener 2012-04-13 11:34:04 UTC
(In reply to comment #35)
> (In reply to comment #34)
> > 
> > btw, why do we print a location info for
> > 
> > t.C:5:6: note: candidates are:
> >    f(1);
> >       ^
> > 
> > at all?
> 
> I am proposing to print just: "note: candidates are:", with enough leading
> empty space to align with the previous error and no caret. Is that OK? Richard,
> Jason?

Sounds good to me.  But I think GNU conventions require a location here?

> > 
> > t.C:1:6: note:   candidate expects 0 arguments, 1 provided
> >  void f();  void f(int,int);
> >       ^
> > t.C:1:17: note: void f(int, int)
> >  void f();  void f(int,int);
> >                  ^
> > 
> > and the 2nd note here looks wrong.
> 
> Could you explain why?

Because void f(int, int) is not of type "candidate expects 0 arguments" but
it is of expects two which is duplicate of the following

t.C:1:17: note:   candidate expects 2 arguments, 1 provided
 void f();  void f(int,int);
                 ^

But that's of course a different bug.
Comment 37 Jonathan Wakely 2012-04-13 11:50:33 UTC
I think for Richard's example a nice compromise would be:

t.C: In function 'int main()':
t.C:5:6: error: no matching function for call to 'f(int)'
   f(1);
      ^
         note: candidates are:
t.C:1:6: note: void f()
t.C:1:6: note:   candidate expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^
t.C:1:17: note: void f(int, int)
t.C:1:17: note:   candidate expects 2 arguments, 1 provided
 void f();  void f(int,int);
                 ^

That includes all the relevant information but removes the duplication caused by printing the same source code and caret after each pair of notes, and the whitespace on the "candidates are" line instead of location info does help make it look less cluttered.  GCC diagnostics do tend to look more densely packed than some other compilers.  Caret diagnostics change that, but it's not necessarily an improvement if they just space out every line with duplicated snippets of source and carets.
Comment 38 Jonathan Wakely 2012-04-13 11:53:31 UTC
(In reply to comment #36)
> > > t.C:1:6: note:   candidate expects 0 arguments, 1 provided
> > >  void f();  void f(int,int);
> > >       ^
> > > t.C:1:17: note: void f(int, int)
> > >  void f();  void f(int,int);
> > >                  ^
> > > 
> > > and the 2nd note here looks wrong.
> > 
> > Could you explain why?
> 
> Because void f(int, int) is not of type "candidate expects 0 arguments" but
> it is of expects two which is duplicate of the following
> 
> t.C:1:17: note:   candidate expects 2 arguments, 1 provided
>  void f();  void f(int,int);
>                  ^

You're confusing two separate notes.

This bit refers to the first overload, which expects 0 args:

t.C:1:6: note:   candidate expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^

And this bit refers to the second overload:

t.C:1:17: note: void f(int, int)
 void f();  void f(int,int);
                 ^

The line following says "expects 2 arguments"

This is why in my previous comment I suggested removing the caret diagnostic between the related notes, so the notes that refer to the same thing are adjacent.
Comment 39 Manuel López-Ibáñez 2012-04-13 11:54:53 UTC
(In reply to comment #36)
> Sounds good to me.  But I think GNU conventions require a location here?

Well, if that is a hard requirement, I can just suppress the caret. Or we can just not print the "note: candidates are:". It seems superfluous info to me.

> > > t.C:1:6: note:   candidate expects 0 arguments, 1 provided
> > >  void f();  void f(int,int);
> > >       ^
> > > t.C:1:17: note: void f(int, int)
> > >  void f();  void f(int,int);
> > >                  ^
> > > 
> > > and the 2nd note here looks wrong.
> > 
> > Could you explain why?
> 
> Because void f(int, int) is not of type "candidate expects 0 arguments" but
> it is of expects two which is duplicate of the following
> 
> t.C:1:17: note:   candidate expects 2 arguments, 1 provided
>  void f();  void f(int,int);
>                  ^
> 
> But that's of course a different bug.

I see. The problem is that the output is meant to be read as (locations and duplicated carets removed for clarity):

note: candidates are:
note: candidate #1 is void f()
 void f();  void f(int,int);
      ^
note:  candidate #1 expects 0 arguments, 1 provided
note: candidate #2 is void f(int, int)
 void f();  void f(int,int);
                 ^
note:   candidate #2 expects 2 arguments, 1 provided

that is, first print the candidate, then the reason for failure. If you read again the original, the 2nd and 3rd notes go together, like the 4th and 5th, so the output is correct (although I agree that quite confusing). That is why I proposed:

note: candidate 'void f()' expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^
note:  candidate 'void f(int, int)' expects 2 arguments, 1 provided
 void f();  void f(int,int);
                 ^

(that is, instead of two notes per candidate, just one). Or we could number the candidates like above.
Comment 40 Manuel López-Ibáñez 2012-04-13 11:58:04 UTC
I think what Jonathan proposed in comment #37 is also nice. If Jason approves, I will implement it.
Comment 41 Jonathan Wakely 2012-04-13 12:07:01 UTC
(In reply to comment #39)
> just not print the "note: candidates are:". It seems superfluous info to me.

Personally I like the "candidates are" line, I don't find it superfluous.

If there are two erroneous calls:

  f(1);
  f(2);

the "candidates are" notes help break up the errors and help me parse them. In real code these lines might be very long and wrap on the screen:

t.cc:1:6: note: void f()
t.cc:1:6: note:   candidate expects 0 arguments, 1 provided

The short, concise "candidates are" line is easy for me to locate quickly and start scanning down the list from there, especially when there is more than one error in the code.
Comment 42 Richard Biener 2012-04-13 12:08:02 UTC
(In reply to comment #40)
> I think what Jonathan proposed in comment #37 is also nice. If Jason approves,
> I will implement it.

Yes, I like that, too.  For reference, the following:

note: candidate 'void f()' expects 0 arguments, 1 provided
 void f();  void f(int,int);
      ^
note:  candidate 'void f(int, int)' expects 2 arguments, 1 provided
 void f();  void f(int,int);
                 ^

I'll approve a patch that implements this.
Comment 43 Manuel López-Ibáñez 2012-04-13 12:15:45 UTC
(In reply to comment #42)
> (In reply to comment #40)
> > I think what Jonathan proposed in comment #37 is also nice. If Jason approves,
> > I will implement it.
> 
> Yes, I like that, too.  For reference, the following:
> 
> note: candidate 'void f()' expects 0 arguments, 1 provided
>  void f();  void f(int,int);
>       ^
> note:  candidate 'void f(int, int)' expects 2 arguments, 1 provided
>  void f();  void f(int,int);
>                  ^

To avoid confusion, that was not what Jonathan proposed in comment #37.

> I'll approve a patch that implements this.

With all due respect, since this is a C++ FE issue, I'll still wait for Jason's opinion before implementing anything. I still appreciate yours (or anyone else) comments. Thanks.
Comment 44 Manuel López-Ibáñez 2012-04-13 12:18:35 UTC
(In reply to comment #41)
> (In reply to comment #39)
> > just not print the "note: candidates are:". It seems superfluous info to me.
> 
> Personally I like the "candidates are" line, I don't find it superfluous.

OK, I don't have a strong opinion on this. Let's Jason decide.

You seem to agree with me on removing the duplicated location in front of it, per comment #37, am I right?
Comment 45 Jonathan Wakely 2012-04-13 12:24:30 UTC
(In reply to comment #42)
> Yes, I like that, too.  For reference, the following:
> 
> note: candidate 'void f()' expects 0 arguments, 1 provided
>  void f();  void f(int,int);
>       ^
> note:  candidate 'void f(int, int)' expects 2 arguments, 1 provided
>  void f();  void f(int,int);
>                  ^

I like this for this example, but does it work as well if the function name is very long, and the "expects 2 arguments, 1 provided" is no longer in a predictable position, but pushed off to the right of a very long line?

t.cc: In function 'int main()':
t.cc:10:25: error: no matching function for call to 'a_long_function_name(int)'
t.cc:10:25: note: candidates are:
t.cc:5:6: note: void a_long_function_name()
t.cc:5:6: note:   candidate expects 0 arguments, 1 provided
t.cc:6:6: note: void a_long_function_name(something_verbose::my_very_long_type, something_verbose::my_very_long_type)
t.cc:6:6: note:   candidate expects 2 arguments, 1 provided

would become

t.cc: In function 'int main()':
t.cc:10:25: error: no matching function for call to 'a_long_function_name(int)'
  a_long_function_name(1);
                        ^
t.cc:10:25: note: candidates are:
t.cc:5:6: note:   candidate 'void a_long_function_name()' expects 0 arguments, 1 provided
 void a_long_function_name();
      ^
t.cc:6:6: note: candidate 'void a_long_function_name(something_verbose::my_very_long_type, something_verbose::my_very_long_type)' expects 2 arguments, 1 provided
 void a_long_function_name(something_verbose::my_very_long_type, something_verbose::my_very_long_type);
      ^

(we do already have this problem when printing ridiculous paths for stdlib headers with superfluous lib/gcc/x86_64-unknown-linux-gnu/4.8.0/../../../.. rubbish in them, is there an existing bug for that?)
Comment 46 Manuel López-Ibáñez 2012-04-13 12:31:41 UTC
(In reply to comment #45)
> (In reply to comment #42)
> > Yes, I like that, too.  For reference, the following:
> > 
> > note: candidate 'void f()' expects 0 arguments, 1 provided
> >  void f();  void f(int,int);
> >       ^
> > note:  candidate 'void f(int, int)' expects 2 arguments, 1 provided
> >  void f();  void f(int,int);
> >                  ^
> 
> I like this for this example, but does it work as well if the function name is
> very long, and the "expects 2 arguments, 1 provided" is no longer in a
> predictable position, but pushed off to the right of a very long line?

I see your point, I am convinced.
 
> (we do already have this problem when printing ridiculous paths for stdlib
> headers with superfluous lib/gcc/x86_64-unknown-linux-gnu/4.8.0/../../../..
> rubbish in them, is there an existing bug for that?)

Please open one with a reproducible testcase and I will take a look. This is very annoying to me as well, and there should be a way to make these paths shorter.
Comment 47 Jason Merrill 2012-04-13 19:04:50 UTC
Jonathan's proposed output looks fine to me.  The "candidates are" note had a source location for the sake of dejagnu, but we can deal with that by adjusting the prune.exp note handling.

But I also want a general solution to the issue of multiple diagnostics with the same source location; overload resolution isn't the only example.

I think suppressing the caret by default for notes makes sense.
Comment 48 Manuel López-Ibáñez 2012-04-13 19:40:53 UTC
(In reply to comment #47)
> Jonathan's proposed output looks fine to me.  The "candidates are" note had a
> source location for the sake of dejagnu, but we can deal with that by adjusting
> the prune.exp note handling.

OK, so I will do that.

> But I also want a general solution to the issue of multiple diagnostics with
> the same source location; overload resolution isn't the only example.

Yes, we should disable the caret if the last caret printed was at the same location.

> I think suppressing the caret by default for notes makes sense.

There are a lot of notes that give useful info with carets. Like "declared here". And notes are used for macro expansion. Anyway, I will have to add some kind of inform_without_locus("") to implement the above, so if we decide to switch all notes to use it, the implementation will be ready.
Comment 49 Manuel López-Ibáñez 2012-04-13 22:59:12 UTC
Created attachment 27155 [details]
fix overload carets

The patch that I am currently bootstrapping. I made a small change to the output. It looks like this now:

caret-overload.C:102:6: error: no matching function for call to ‘f(int)’
   f(1);
      ^
                        note: candidates are:
caret-overload.C:1:6: note: void f()
                      note:   candidate expects 0 arguments, 1 provided
 void f();
      ^
caret-overload.C:10:6: note: void f(int, int)
                       note:   candidate expects 2 arguments, 1 provided
 void f(int,int);
      ^


But if you don't like it, I can switch off the DI_HIDE_PREFIX for the failure reason. Note that the notes without prefix will be perfectly aligned with the previous message.
Comment 50 Manuel López-Ibáñez 2012-04-14 10:53:25 UTC
(In reply to comment #49)
> Created attachment 27155 [details]
> fix overload carets
> 
> The patch that I am currently bootstrapping. I made a small change to the
> output. It looks like this now:

It bootstraps but, of course, there are a lot of failures. This one:

>                         note: candidates are:

I can remove in prune.exp but this:

>                       note:   candidate expects 0 arguments, 1 provided

We probably want to match in messages. I am pretty sure there is a way to add a new { dg-notes-2 "note1" "note2" } which passes a regexp such as "[^\n]*note1[^\n]*\n[^\n]*note2[^\n]*" to process-message, but my Tcl foo is not strong enough.
Comment 51 Manuel López-Ibáñez 2012-04-22 16:35:34 UTC
BTW, if you have more examples where printing the caret seems unnecessary, please open PRs and add me to the CC. I will try to deal with them and hopefully the caret will stay enabled by default for GCC 4.8.
Comment 52 Manuel López-Ibáñez 2012-05-04 00:31:58 UTC
Author: manu
Date: Fri May  4 00:31:55 2012
New Revision: 187134

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187134
Log:
2012-05-04  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c++/24985
gcc/
	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc): Show caret
	for macro expansion.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-diagnostic.c
Comment 53 Manuel López-Ibáñez 2012-05-13 22:32:19 UTC
Well, I would consider this fixed in 4.8. Specific caret issues should have their own PRs.