[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #53 from Manuel López-Ibáñez manu at gcc dot gnu.org 2012-05-13 22:32:19 UTC --- Well, I would consider this fixed in 4.8. Specific caret issues should have their own PRs.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #52 from Manuel López-Ibáñez manu at gcc dot gnu.org 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=gccview=revrev=187134 Log: 2012-05-04 Manuel López-Ibáñez m...@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
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #51 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
--- Comment #50 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
--- Comment #34 from Richard Guenther rguenth at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #35 from Manuel López-Ibáñez manu at gcc dot gnu.org 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?
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #36 from Richard Guenther rguenth at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #37 from Jonathan Wakely redi at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #38 from Jonathan Wakely redi at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #39 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #40 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #41 from Jonathan Wakely redi at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #42 from Richard Guenther rguenth at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #43 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #44 from Manuel López-Ibáñez manu at gcc dot gnu.org 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?
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #45 from Jonathan Wakely redi at gcc dot gnu.org 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?)
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #46 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #47 from Jason Merrill jason at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #48 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added Attachment #27089|0 |1 is obsolete|| Attachment #27093|0 |1 is obsolete|| --- Comment #49 from Manuel López-Ibáñez manu at gcc dot gnu.org 2012-04-13 22:59:12 UTC --- Created attachment 27155 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=27155 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
--- Comment #31 from Jason Merrill jason at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #32 from Manuel López-Ibáñez manu at gcc dot gnu.org 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?
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #33 from Paolo Carlini paolo.carlini at oracle dot com 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' ;)
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #29 from Manuel López-Ibáñez manu at gcc dot gnu.org 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=gccview=revrev=186305 Log: 2012-04-11 Manuel López-Ibáñez m...@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
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #30 from Manuel López-Ibáñez manu at gcc dot gnu.org 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!
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #27 from dodji at seketeli dot org dodji at seketeli dot org 2012-04-09 16:04:46 UTC --- manu at gcc dot gnu.org gcc-bugzi...@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
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #28 from dodji at seketeli dot org dodji at seketeli dot org 2012-04-09 16:19:15 UTC --- manu at gcc dot gnu.org gcc-bugzi...@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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
Manuel López-Ibáñez manu at gcc dot gnu.org changed:
What|Removed |Added
CC||dodji at gcc dot gnu.org
--- Comment #26 from Manuel López-Ibáñez manu at gcc dot gnu.org 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?
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #22 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #23 from Jason Merrill jason at gcc dot gnu.org 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\].
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #24 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #25 from Manuel López-Ibáñez manu at gcc dot gnu.org 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 ?
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||jason at gcc dot gnu.org --- Comment #16 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #17 from Manuel López-Ibáñez manu at gcc dot gnu.org 2012-04-04 15:28:08 UTC --- Created attachment 27089 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=27089 caret diagnostics patch
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Paolo Carlini paolo.carlini at oracle dot com changed: What|Removed |Added CC||paolo.carlini at oracle dot ||com --- Comment #18 from Paolo Carlini paolo.carlini at oracle dot com 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)
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #19 from Jason Merrill jason at gcc dot gnu.org 2012-04-04 18:47:58 UTC --- Created attachment 27092 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=27092 Patch to add -fno-diagnostics-show-caret for testing This seems like what you want for the last bit.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 --- Comment #20 from Manuel López-Ibáñez manu at gcc dot gnu.org 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Jason Merrill jason at gcc dot gnu.org changed: What|Removed |Added Attachment #27092|0 |1 is obsolete|| --- Comment #21 from Jason Merrill jason at gcc dot gnu.org 2012-04-04 19:19:21 UTC --- Created attachment 27093 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=27093 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.
[Bug c++/24985] caret diagnostics
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985 Manuel López-Ibáñez manu at gcc dot gnu.org changed: What|Removed |Added CC||borut.razem at gmail dot ||com --- Comment #15 from Manuel López-Ibáñez manu at gcc dot gnu.org 2012-03-21 12:05:08 UTC --- *** Bug 52655 has been marked as a duplicate of this bug. ***
[Bug c++/24985] caret diagnostics
--- Comment #13 from bkoz at gcc dot gnu dot org 2009-10-15 17:33 --- Patch for change to coding conventions: http://gcc.gnu.org/ml/gcc-patches/2009-10/msg00687.html -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #14 from manu at gcc dot gnu dot org 2009-10-15 18:51 --- 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 -- manu at gcc dot gnu dot org changed: What|Removed |Added URL||http://gcc.gnu.org/wiki/Bett ||er_Diagnostics http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #9 from bkoz at gcc dot gnu dot org 2009-10-07 16:56 --- Beyond caret diagnostics there is also range info for the diagnostic location. See: http://clang.llvm.org/diagnostics.html -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #10 from manu at gcc dot gnu dot org 2009-10-07 17:06 --- (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. -- manu at gcc dot gnu dot org changed: What|Removed |Added Last reconfirmed|2006-02-20 18:43:14 |2009-10-07 17:06:57 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #11 from jason at gcc dot gnu dot org 2009-10-07 18:31 --- (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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #12 from manu at gcc dot gnu dot org 2009-10-07 21:25 --- (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 -- manu at gcc dot gnu dot org changed: What|Removed |Added Last reconfirmed|2009-10-07 17:06:57 |2009-10-07 21:25:54 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #8 from dave at treblig dot org 2009-07-03 11:03 --- 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 -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #4 from manu at gcc dot gnu dot org 2009-07-02 23:49 --- *** Bug 40228 has been marked as a duplicate of this bug. *** -- manu at gcc dot gnu dot org changed: What|Removed |Added CC||dave at treblig dot org http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #5 from paolo dot carlini at oracle dot com 2009-07-02 23:55 --- 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 ;) -- paolo dot carlini at oracle dot com changed: What|Removed |Added CC||paolo at gcc dot gnu dot org http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #6 from manu at gcc dot gnu dot org 2009-07-03 00:25 --- (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*. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #7 from paolo dot carlini at oracle dot com 2009-07-03 00:38 --- I see, thanks Manuel for the feedback, I'll give it some thought. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
[Bug c++/24985] caret diagnostics
--- Comment #3 from manu at gcc dot gnu dot org 2008-07-22 09:50 --- Better summary to find duplicates. -- manu at gcc dot gnu dot org changed: What|Removed |Added Alias||caret Summary|Line info in diagnostics is |caret diagnostics |very unfriendly | http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24985
