| Message ID | 538F7856.1090104@debian.org |
|---|---|
| State | New |
| Headers | show |
On Jun 4, 2014, at 12:49 PM, Sylvestre Ledru <sylvestre@debian.org> wrote: > Finally, I have been able to update all tests with -Wreturn-type enabled > by default. > Now, I would like to know if I can commit that into the repository. Who > can review that? I’d like a C style person to review gcc.dg/Wreturn-type2.c, just to ensure they think it is reasonable, and a Fortran person to spot check, but other than that the test suite patches look good. I will note that adding: +/* { dg-options "-Wno-return-type" } */ to some parts of the test suite will change the options used to compile: # If a testcase doesn't have special options, use these. global DEFAULT_CXXFLAGS if ![info exists DEFAULT_CXXFLAGS] then { set DEFAULT_CXXFLAGS " -pedantic-errors -Wno-long-long” } In theory, this could matter. I was going to not worry about this problem, and let people fault the additional options in as necessary. Also, any time there are target options, it is easy to miss adding the flag to the other dg-options lines and have them by caught by your test suite runs, but I checked and you seemed to have correctly edited them, though I only spot checked.
On Wed, 4 Jun 2014, Sylvestre Ledru wrote: > Hello, > > Finally, I have been able to update all tests with -Wreturn-type enabled > by default. AFAIK, under GNU/Linux Debian Jessie 64 bits, there is no > PASS->FAIL tests. > > Now, I would like to know if I can commit that into the repository. Who > can review that? > > As attachment, you will find the actual (tiny) patch. > > I split the tests update by languages. As they are big ( 1260 files > changed, 1638 insertions(+), 903 deletions(-) ), I uploaded the patches > on my server: Some of those patches appear to be addressing cases where control appears to reach the end of a function returning non-void, as opposed to cases where the return type defaults to int. As I said in <https://gcc.gnu.org/ml/gcc/2014-01/msg00207.html>, I don't think that warning is appropriate to enable by default as it catches perfectly valid C90 / C99 code that avoids using extensions to annotate noreturn functions. (I *do* think it's appropriate to enable by default the warning about return type defaulting to int - more generally, to enable -Wimplicit-int -Wimplicit-function-declaration - and the -Wreturn-type warning about a return statement without a value in a function returning non-void also seems appropriate to enable by default. Warning about the absence of any return statement in a function returning non-void is probably also a reasonable default warning from the -Wreturn-type set; it's specifically the flow-based warnings that can give false positives in the absence of noreturn annotations that I'm dubious about enabling by default.)
On 05/06/2014 01:31, Joseph S. Myers wrote: > On Wed, 4 Jun 2014, Sylvestre Ledru wrote: > >> Hello, >> >> Finally, I have been able to update all tests with -Wreturn-type enabled >> by default. AFAIK, under GNU/Linux Debian Jessie 64 bits, there is no >> PASS->FAIL tests. >> >> Now, I would like to know if I can commit that into the repository. Who >> can review that? >> >> As attachment, you will find the actual (tiny) patch. >> >> I split the tests update by languages. As they are big ( 1260 files >> changed, 1638 insertions(+), 903 deletions(-) ), I uploaded the patches >> on my server: > > Some of those patches appear to be addressing cases where control appears > to reach the end of a function returning non-void, as opposed to cases > where the return type defaults to int. Do you have an example of the patches you are talking about? > As I said in > <https://gcc.gnu.org/ml/gcc/2014-01/msg00207.html>, I don't think that > warning is appropriate to enable by default as it catches perfectly valid > C90 / C99 code that avoids using extensions to annotate noreturn > functions. I can try to implement that but I don't know where to start. Any clue? > (I *do* think it's appropriate to enable by default the warning about > return type defaulting to int - more generally, to enable -Wimplicit-int > -Wimplicit-function-declaration - and the -Wreturn-type warning about a > return statement without a value in a function returning non-void also > seems appropriate to enable by default. I can try to enable them too by default. It seems my patches are covering most of the tests updates. > Warning about the absence of any > return statement in a function returning non-void is probably also a > reasonable default warning from the -Wreturn-type set; it's specifically > the flow-based warnings that can give false positives in the absence of > noreturn annotations that I'm dubious about enabling by default.) You are talking about code like this one (from Jonathan Wakely) ? int f(int c) { if (c) return 0; function_that_never_returns(); } Initially, I implemented -Wmissing-return to manage this case ( https://gcc.gnu.org/ml/gcc-patches/2014-01/msg00820.html ) but Jason suggested to remove that: https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01033.html (I don't have a strong opinion on the subject). Sylvestre
On Thu, 5 Jun 2014, Sylvestre Ledru wrote: > > Some of those patches appear to be addressing cases where control appears > > to reach the end of a function returning non-void, as opposed to cases > > where the return type defaults to int. > Do you have an example of the patches you are talking about? In 0004-Update-gcc-tests-with-warning-return-type-enabled-by.patch the very first change is adding such a "return 0;" (as are lots of others). > You are talking about code like this one (from Jonathan Wakely) ? > > int f(int c) > { > if (c) > return 0; > function_that_never_returns(); > } Yes. > Initially, I implemented -Wmissing-return to manage this case ( > https://gcc.gnu.org/ml/gcc-patches/2014-01/msg00820.html ) but Jason > suggested to remove that: > https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01033.html > (I don't have a strong opinion on the subject). I think splitting the option like that makes sense. Compatibility indicates that -Wreturn-type and -Wall should still enable -Wmissing-return, but only the other pieces of -Wreturn-type should be enabled by default, at least for C. (Enabling -Wimplicit-int by default might be a good starting point.) Also, at least one testsuite change in your patch is wrong. You add an "int" return type to c90-impl-int-1.c, which is explicitly checking the implicit int functionality for C90; use of dg-warning there would be more appropriate (since the point is that it doesn't give an error with -pedantic-errors). It would probably also be best not to add -Wno-return-type in c99-impl-int-1.c. (Any places where /* { dg-bogus "warning" "warning in place of error" } */ in tests causes problems because you get a new warning *in addition* to the existing error can have that dg-bogus removed and a dg-warning directive for the warning added - dg-warning/dg-error used not to distinguish properly between warnings and errors, so requiring such dg-bogus directives if you wanted to test the difference, but that was fixed a long time ago.)
gcc/c-family/ChangeLog: 2014-06-04 Sylvestre Ledru <sylvestre@debian.org> * c.opt: -Wreturn-type enabled by default From 650edb9943ba8b2afb4995e70f671d8fdc26e10a Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru <sylvestre@debian.org> Date: Wed, 4 Jun 2014 13:33:40 +0200 Subject: [PATCH 1/6] Enable warning return-type by default --- gcc/c-family/c.opt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 5d36a80..8e78a9d 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -686,7 +686,7 @@ C ObjC C++ ObjC++ Var(warn_return_local_addr) Init(1) Warning Warn about returning a pointer/reference to a local or temporary variable. Wreturn-type -C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +C ObjC C++ ObjC++ Var(warn_return_type) Init(1) Warning Warn whenever a function's return type defaults to \"int\" (C), or about inconsistent return types (C++) Wselector -- 2.0.0