| Message ID | 20150929155514.GG6184@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 29 Sep 2015, Marek Polacek wrote: > This fixes missing warning for the attached testcase. In such a case, > we must use the expansion point location. I didn't simply add > loc = expansion_point_location_if_in_system_header (loc); > as might be seen elsewhere in the codebase because we pass LOC down to > convert_for_assignment where many of the warnings are issued and I was > nervous about passing a different location there. I assume this means that the other missing warning from http://stackoverflow.com/questions/32732281/no-warning-when-returning-null-with-gcc (same code but change the return type from void to int) is not fixed at the same time?
On Tue, Sep 29, 2015 at 06:04:55PM +0200, Marc Glisse wrote: > On Tue, 29 Sep 2015, Marek Polacek wrote: > > >This fixes missing warning for the attached testcase. In such a case, > >we must use the expansion point location. I didn't simply add > > loc = expansion_point_location_if_in_system_header (loc); > >as might be seen elsewhere in the codebase because we pass LOC down to > >convert_for_assignment where many of the warnings are issued and I was > >nervous about passing a different location there. > > I assume this means that the other missing warning from > http://stackoverflow.com/questions/32732281/no-warning-when-returning-null-with-gcc > (same code but change the return type from void to int) > is not fixed at the same time? Nope, I wasn't aware of that one :(. Maybe we want the loc = expansion_point_location_if_in_system_header (loc); line after all... Marek
On Tue, 29 Sep 2015, Marek Polacek wrote: > This fixes missing warning for the attached testcase. In such a case, > we must use the expansion point location. I didn't simply add > loc = expansion_point_location_if_in_system_header (loc); > as might be seen elsewhere in the codebase because we pass LOC down to > convert_for_assignment where many of the warnings are issued and I was > nervous about passing a different location there. I suppose that for the convert_for_assignment cases you should warn if the user's code is in any way responsible for the issue, which includes if a user's function returns a wrong-type macro defined in a system header (or for that matter if a system header contains a return but the user chose the argument to that return, but that seems much less likely). > Bootstrapped/regtested on x86_64-linux, ok for trunk and 5? OK (though followups may be needed for any other issues).
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index 3b26231..a11ccb2 100644 --- gcc/c/c-typeck.c +++ gcc/c/c-typeck.c @@ -9369,8 +9369,12 @@ c_finish_return (l_cation_t ttt, tree retval, tree origtype) bool npc = false; size_t rank = 0; + /* Use the expansion point to handle cases such as returning NULL + in a function returning void. */ + source_location xloc = expansion_point_location_if_in_system_header (loc); + if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (loc, 0, + warning_at (xloc, 0, "function declared %<noreturn%> has a %<return%> statement"); if (flag_cilkplus && contains_array_notation_expr (retval)) @@ -9425,10 +9429,10 @@ c_finish_return (location_t loc, tree retval, tree origtype) { current_function_returns_null = 1; if (TREE_CODE (TREE_TYPE (retval)) != VOID_TYPE) - pedwarn (loc, 0, + pedwarn (xloc, 0, "%<return%> with a value, in function returning void"); else - pedwarn (loc, OPT_Wpedantic, "ISO C forbids " + pedwarn (xloc, OPT_Wpedantic, "ISO C forbids " "%<return%> with expression, in function returning void"); } else diff --git gcc/testsuite/gcc.dg/pr67730.c gcc/testsuite/gcc.dg/pr67730.c index e69de29..54d73a6 100644 --- gcc/testsuite/gcc.dg/pr67730.c +++ gcc/testsuite/gcc.dg/pr67730.c @@ -0,0 +1,11 @@ +/* PR c/67730 */ +/* { dg-do compile } */ +/* { dg-options "" } */ + +#include <stddef.h> + +void +fn1 (void) +{ + return NULL; /* { dg-warning "10:.return. with a value" } */ +}