Re: [Patch] PR55189 enable -Wreturn-type by default
On 20/08/2014 00:02, Joseph S. Myers wrote: On Fri, 15 Aug 2014, Sylvestre Ledru wrote: It is indeed useless. I removed it. Thanks http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch I don't think most of the testsuite changes in this patch should be needed, and we should be conservative about changing existing testcases because of the risk that it affects what they test. Most of the changes seem like they would only have been relevant for the previous version that enabled -Wmissing-return warnings by default. The change to gcc.dg/c90-impl-int-1.c is simply wrong - the specific point of that testcase is to test various cases of implicit int, so you can't add explicit int return types to it. You need, obviously, the new tests for how -W(no-)missing-return and -W(no-)return-type work and what the defaults are. Existing tests should only need to be changed if they do in fact fail with the compiler patch applied. Thanks for the feedback. I updated the patch (including the gcc.dg/c90-impl-int-1.c change): http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch For information, the number of files modified by this commit dropped from 1298 to 818. Thanks, Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
On Wed, 20 Aug 2014, Sylvestre Ledru wrote: On 20/08/2014 00:02, Joseph S. Myers wrote: On Fri, 15 Aug 2014, Sylvestre Ledru wrote: It is indeed useless. I removed it. Thanks http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch I don't think most of the testsuite changes in this patch should be needed, and we should be conservative about changing existing testcases because of the risk that it affects what they test. Most of the changes seem like they would only have been relevant for the previous version that enabled -Wmissing-return warnings by default. The change to gcc.dg/c90-impl-int-1.c is simply wrong - the specific point of that testcase is to test various cases of implicit int, so you can't add explicit int return types to it. You need, obviously, the new tests for how -W(no-)missing-return and -W(no-)return-type work and what the defaults are. Existing tests should only need to be changed if they do in fact fail with the compiler patch applied. Thanks for the feedback. I updated the patch (including the gcc.dg/c90-impl-int-1.c change): http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch For information, the number of files modified by this commit dropped from 1298 to 818. Are you sure that's the right patch? * It seems to be missing the new testcases, which are a necessary part of the patch. * The first few testcase changes I looked at in the patch still don't make sense to me. They are changing int return types to void, or adding return 0;, which should not be needed with the defaults as I understand them - the warnings for those should be the control reaches end of non-void function which I thought was being left off by default. If those are getting warnings (for C) without the changes, there's something I still don't understand about what this patch is doing and why it's meant to be safe. Could you clarify - in terms of source code constructs, not option names - which cases your patch would or would not cause to receive warnings by default for C? -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On Fri, 15 Aug 2014, Sylvestre Ledru wrote: It is indeed useless. I removed it. Thanks http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch I don't think most of the testsuite changes in this patch should be needed, and we should be conservative about changing existing testcases because of the risk that it affects what they test. Most of the changes seem like they would only have been relevant for the previous version that enabled -Wmissing-return warnings by default. The change to gcc.dg/c90-impl-int-1.c is simply wrong - the specific point of that testcase is to test various cases of implicit int, so you can't add explicit int return types to it. You need, obviously, the new tests for how -W(no-)missing-return and -W(no-)return-type work and what the defaults are. Existing tests should only need to be changed if they do in fact fail with the compiler patch applied. -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On 14/08/2014 20:48, Manuel López-Ibáñez wrote:
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -693,6 +693,10 @@ gfc_handle_option (size_t scode, const char *arg,
int value,
gfc_option.warn_line_truncation = value;
break;
+case OPT_Wmissing_return:
+ warn_missing_return = value;
+ break;
+
case OPT_Wrealloc_lhs:
gfc_option.warn_realloc_lhs = value;
break;
The entry in c.opt says this is a C/C++ option, why you need this?
It is indeed useless. I removed it. Thanks
http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -472,7 +472,7 @@ C ObjC Var(warn_implicit_function_declaration)
Init(-1) Warning LangEnabledBy(C
Warn about implicit function declarations
Wimplicit-int
-C ObjC Var(warn_implicit_int) Warning LangEnabledBy(C ObjC,Wimplicit)
+C ObjC Var(warn_implicit_int) Warning
Warn when a declaration does not specify a type
Wimport
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5ae910c..3f2019a 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3615,7 +3615,7 @@ This warning is enabled by @option{-Wall} in C++.
@opindex Wimplicit-int
@opindex Wno-implicit-int
Warn when a declaration does not specify a type.
-This warning is enabled by @option{-Wall}.
+This warning is enabled by default.
@item -Wimplicit-function-declaration @r{(C and Objective-C only)}
@opindex Wimplicit-function-declaration
Does this patch actually enables -Wimplicit-int by default? The
default without Init() should be zero!
And according to this:
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01367.html
we still want -Wno-implicit to disable -Wimplicit-int (and
-Werror=implicit to set -Werror=implicit-int), so the LangEnabledBy()
should stay. The documentation could say: This warning is enabled by
default and it is also controlled by -Wimplicit.
OK. I will go back on this once the first patch is committed.
Thanks,
Sylvestre
Full changelog:
gcc/c-family/ChangeLog:
2014-08-13 Sylvestre Ledru sylves...@debian.org
* c.opt: Enable -Wreturn-type by default
Add -Wmissing-return:
Warn whenever control may reach end of non-void function
gcc/ChangeLog:
2014-08-13 Sylvestre Ledru sylves...@debian.org
* doc/invoke.texi: Document new flag -Wmissing-return
Update -Wreturn-type
* tree-cfg.c (pass_warn_function_return::execute):
Introduce -Wreturn-type management
libgomp/ChangeLog:
2014-08-13 Sylvestre Ledru sylves...@debian.org
* testsuite/libgomp.c++/loop-2.C: Update the test with -Wreturn-type by
default and -Wmissing-return
* testsuite/libgomp.c++/loop-4.C: likewise
* testsuite/libgomp.c++/parallel-1.C: likewise
* testsuite/libgomp.c++/shared-1.C: likewise
* testsuite/libgomp.c++/single-1.C: likewise
* testsuite/libgomp.c++/single-2.C: likewise
* testsuite/libgomp.c/omp-loop02.c: likewise
* testsuite/libgomp.c/omp-parallel-for.c: likewise
* testsuite/libgomp.c/omp-parallel-if.c: likewise
* testsuite/libgomp.c/omp-single-1.c: likewise
* testsuite/libgomp.c/omp-single-2.c: likewise
* testsuite/libgomp.c/omp_matvec.c: likewise
* testsuite/libgomp.c/omp_workshare3.c: likewise
* testsuite/libgomp.c/omp_workshare4.c: likewise
* testsuite/libgomp.c/pr30494.c (check): likewise
* testsuite/libgomp.c/shared-1.c: likewise
gcc/testsuite/ChangeLog:
2014-08-13 Sylvestre Ledru sylves...@debian.org
* gcc.dg/Wmissing-return1.c: New test which tests the new behavior
* gcc.dg/Wmissing-return2.c: New test which tests the new behavior
* gcc.dg/Wmissing-return3.c: New test which tests the new behavior
* gcc.dg/Wmissing-return4.c: New test which tests the new behavior
* gcc.dg/Wmissing-return5.c: New test which tests the new behavior
* c-c++-common/asan/no-redundant-instrumentation-2.c (main):
Update the test with -Wreturn-type by default and -Wmissing-return
* c-c++-common/cilk-plus/AN/decl-ptr-colon.c (int main): likewise
* c-c++-common/cilk-plus/AN/parser_errors.c: likewise
* c-c++-common/cilk-plus/AN/parser_errors2.c: likewise
* c-c++-common/cilk-plus/AN/parser_errors3.c: likewise
* c-c++-common/cilk-plus/AN/pr57457-2.c: likewise
* c-c++-common/cilk-plus/AN/pr57541-2.c (void foo1): likewise
(void foo2): likewise
* c-c++-common/cilk-plus/AN/pr57541.c (int foo): likewise
(int foo1): likewise
* c-c++-common/cilk-plus/CK/pr60197.c: likewise
* c-c++-common/cilk-plus/CK/spawn_in_return.c: likewise
* c-c++-common/convert-vec-1.c: likewise
* c-c++-common/dfp/call-by-value.c (int foo32): likewise
(int foo64): likewise
(int foo128): likewise
* c-c++-common/pr36513-2.c (int main2): likewise
* c-c++-common/pr36513.c (int main1): likewise
* c-c++-common/pr43772.c: likewise
* c-c++-common/pr49706-2.c (same): likewise
Re: [Patch] PR55189 enable -Wreturn-type by default
On 12/08/2014 19:48, Joseph S. Myers wrote: On Mon, 11 Aug 2014, Sylvestre Ledru wrote: The test Wmissing-return2.c only has one of the two warnings. But as per -Wreturn-type = Run both, and for backwards compatibility with the existing definition of -Wreturn-type, both warnings should appear for this test. Make sense. Thanks for the feedback and the help. Here it is: https://github.com/sylvestre/gcc/commit/089ffc9fb85034111b892ee10190dc12b5dbe551 Yes, those tests seem as I expect. So, here is the full commit now. Since the patch is 600k, I uploaded it here: http://sylvestre.ledru.info/0001-Enable-warning-Wreturn-type-by-default.patch Let me know when I can commit that. At the end of this mail, I also added the changelog for Enable -Wimplicit-int by default Patch: http://sylvestre.ledru.info/0002-Enable-Wimplicit-int-by-default.patch Let me know if you prefer a separate mail. Everything is on github if it helps: https://github.com/sylvestre/gcc gcc/c-family/ChangeLog: 2014-08-13 Sylvestre Ledru sylves...@debian.org * c.opt: Enable -Wreturn-type by default Add -Wmissing-return: Warn whenever control may reach end of non-void function gcc/ChangeLog: 2014-08-13 Sylvestre Ledru sylves...@debian.org * doc/invoke.texi: Document new flag -Wmissing-return Update -Wreturn-type * tree-cfg.c (pass_warn_function_return::execute): Introduce -Wreturn-type management gcc/fortran/ChangeLog: 2014-08-13 Sylvestre Ledru sylves...@debian.org * options.c (gfc_handle_option): Manage the new flag -Wmissing-return libgomp/ChangeLog: 2014-08-13 Sylvestre Ledru sylves...@debian.org * testsuite/libgomp.c++/loop-2.C: Update the test with -Wreturn-type by default and -Wmissing-return * testsuite/libgomp.c++/loop-4.C: likewise * testsuite/libgomp.c++/parallel-1.C: likewise * testsuite/libgomp.c++/shared-1.C: likewise * testsuite/libgomp.c++/single-1.C: likewise * testsuite/libgomp.c++/single-2.C: likewise * testsuite/libgomp.c/omp-loop02.c: likewise * testsuite/libgomp.c/omp-parallel-for.c: likewise * testsuite/libgomp.c/omp-parallel-if.c: likewise * testsuite/libgomp.c/omp-single-1.c: likewise * testsuite/libgomp.c/omp-single-2.c: likewise * testsuite/libgomp.c/omp_matvec.c: likewise * testsuite/libgomp.c/omp_workshare3.c: likewise * testsuite/libgomp.c/omp_workshare4.c: likewise * testsuite/libgomp.c/pr30494.c (check): likewise * testsuite/libgomp.c/shared-1.c: likewise gcc/testsuite/ChangeLog: 2014-08-13 Sylvestre Ledru sylves...@debian.org * gcc.dg/Wmissing-return1.c: New test which tests the new behavior * gcc.dg/Wmissing-return2.c: New test which tests the new behavior * gcc.dg/Wmissing-return3.c: New test which tests the new behavior * gcc.dg/Wmissing-return4.c: New test which tests the new behavior * gcc.dg/Wmissing-return5.c: New test which tests the new behavior * c-c++-common/asan/no-redundant-instrumentation-2.c (main): Update the test with -Wreturn-type by default and -Wmissing-return * c-c++-common/cilk-plus/AN/decl-ptr-colon.c (int main): likewise * c-c++-common/cilk-plus/AN/parser_errors.c: likewise * c-c++-common/cilk-plus/AN/parser_errors2.c: likewise * c-c++-common/cilk-plus/AN/parser_errors3.c: likewise * c-c++-common/cilk-plus/AN/pr57457-2.c: likewise * c-c++-common/cilk-plus/AN/pr57541-2.c (void foo1): likewise (void foo2): likewise * c-c++-common/cilk-plus/AN/pr57541.c (int foo): likewise (int foo1): likewise * c-c++-common/cilk-plus/CK/pr60197.c: likewise * c-c++-common/cilk-plus/CK/spawn_in_return.c: likewise * c-c++-common/convert-vec-1.c: likewise * c-c++-common/dfp/call-by-value.c (int foo32): likewise (int foo64): likewise (int foo128): likewise * c-c++-common/pr36513-2.c (int main2): likewise * c-c++-common/pr36513.c (int main1): likewise * c-c++-common/pr43772.c: likewise * c-c++-common/pr49706-2.c (same): likewise * c-c++-common/raw-string-3.c: likewise * c-c++-common/tm/pr54893.c: likewise * c-c++-common/tm/trxn-expr-2.c: likewise * c-c++-common/tsan/fd_pipe_race.c: likewise * c-c++-common/tsan/tls_race.c: likewise * c-c++-common/vector-1.c (int f): likewise * c-c++-common/vector-2.c (void f): likewise * g++.dg/abi/covariant2.C (struct c3): likewise (struct c7): likewise * g++.dg/abi/covariant3.C: likewise * g++.dg/abi/key2.C (int sub): likewise * g++.dg/abi/mangle7.C: likewise * g++.dg/bprob/g++-bprob-1.C (call_for): likewise * g++.dg/cilk-plus/AN/builtin_fn_mutating_tplt.cc: likewise * g++.dg/conversion/op1.C (class C): likewise (int fn): likewise * g++.dg/conversion/op6.C: likewise * g++.dg/cpp0x/access01.C: likewise * g++.dg/cpp0x/alias-decl-19.C: likewise * g++.dg/cpp0x/auto2.C (struct A): likewise * g++.dg/cpp0x/constexpr-defarg2.C: likewise *
Re: [Patch] PR55189 enable -Wreturn-type by default
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -693,6 +693,10 @@ gfc_handle_option (size_t scode, const char *arg,
int value,
gfc_option.warn_line_truncation = value;
break;
+case OPT_Wmissing_return:
+ warn_missing_return = value;
+ break;
+
case OPT_Wrealloc_lhs:
gfc_option.warn_realloc_lhs = value;
break;
The entry in c.opt says this is a C/C++ option, why you need this?
+Wmissing-return
+C ObjC C++ ObjC++ Var(warn_missing_return) LangEnabledBy(C ObjC C++
ObjC++,Wreturn-type)
+Warn whenever control may reach end of non-void function
This should prevent that using -Wreturn-type in Fortran tries to
enable -Wmissing-return, if not that is a bug.
In any case, the work-around should be adding Wmissing-return to
fortran/lang.opt with a ??? comment, not there.
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -472,7 +472,7 @@ C ObjC Var(warn_implicit_function_declaration)
Init(-1) Warning LangEnabledBy(C
Warn about implicit function declarations
Wimplicit-int
-C ObjC Var(warn_implicit_int) Warning LangEnabledBy(C ObjC,Wimplicit)
+C ObjC Var(warn_implicit_int) Warning
Warn when a declaration does not specify a type
Wimport
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5ae910c..3f2019a 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3615,7 +3615,7 @@ This warning is enabled by @option{-Wall} in C++.
@opindex Wimplicit-int
@opindex Wno-implicit-int
Warn when a declaration does not specify a type.
-This warning is enabled by @option{-Wall}.
+This warning is enabled by default.
@item -Wimplicit-function-declaration @r{(C and Objective-C only)}
@opindex Wimplicit-function-declaration
Does this patch actually enables -Wimplicit-int by default? The
default without Init() should be zero!
And according to this: https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01367.html
we still want -Wno-implicit to disable -Wimplicit-int (and
-Werror=implicit to set -Werror=implicit-int), so the LangEnabledBy()
should stay. The documentation could say: This warning is enabled by
default and it is also controlled by -Wimplicit.
Cheers,
Manuel.
Re: [Patch] PR55189 enable -Wreturn-type by default
On Mon, 11 Aug 2014, Sylvestre Ledru wrote: The test Wmissing-return2.c only has one of the two warnings. But as per -Wreturn-type = Run both, and for backwards compatibility with the existing definition of -Wreturn-type, both warnings should appear for this test. Make sense. Thanks for the feedback and the help. Here it is: https://github.com/sylvestre/gcc/commit/089ffc9fb85034111b892ee10190dc12b5dbe551 Yes, those tests seem as I expect. By the way, do you prefer a single commit for all tests or one per directory (gfortran, C++, gcc, OpenMP) ? Each commit needs to avoid causing regressions. Thus, if a change to compiler behavior would cause an existing test to fail, the change to the test needs to be in the same commit as the change of behavior - or in a previous commit if the changed test works both with and without the compiler change. -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On 12/08/2014 19:48, Joseph S. Myers wrote: On Mon, 11 Aug 2014, Sylvestre Ledru wrote: The test Wmissing-return2.c only has one of the two warnings. But as per -Wreturn-type = Run both, and for backwards compatibility with the existing definition of -Wreturn-type, both warnings should appear for this test. Make sense. Thanks for the feedback and the help. Here it is: https://github.com/sylvestre/gcc/commit/089ffc9fb85034111b892ee10190dc12b5dbe551 Yes, those tests seem as I expect. Thanks! By the way, do you prefer a single commit for all tests or one per directory (gfortran, C++, gcc, OpenMP) ? Each commit needs to avoid causing regressions. Thus, if a change to compiler behavior would cause an existing test to fail, the change to the test needs to be in the same commit as the change of behavior - or in a previous commit if the changed test works both with and without the compiler change. Since -Wreturn -Wmissing-return are tied, I will have to commit everything at once. I am going to send the patch asap (after an update + build + tests to make sure new tests pass). Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
On 31/07/2014 00:08, Joseph S. Myers wrote: On Mon, 7 Jul 2014, Sylvestre Ledru wrote: Hello, On 17/06/2014 19:41, Joseph S. Myers wrote: On Tue, 17 Jun 2014, Sylvestre Ledru wrote: OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? That looks like the right things to test, if there are no changes for anything other than those options. Here it is: https://github.com/sylvestre/gcc/commit/db8aaac91aa09fd1ec1cc8974586aec45a221e71 Is that what you expected? The test Wmissing-return2.c only has one of the two warnings. But as per -Wreturn-type = Run both, and for backwards compatibility with the existing definition of -Wreturn-type, both warnings should appear for this test. Make sense. Thanks for the feedback and the help. Here it is: https://github.com/sylvestre/gcc/commit/089ffc9fb85034111b892ee10190dc12b5dbe551 (It's simply that only a subset of -Wreturn-type seems suitable to enable by default for C. Maybe the default subset, -Wreturn-type -Wno-missing-return (which is a combination that should also be included in the testcases), should have its own option name, although I don't know what that would be.) What about implementing that in an further commit? I have touched more than 1200 test files and I would like to see that merged soon to avoid more conflicts. By the way, do you prefer a single commit for all tests or one per directory (gfortran, C++, gcc, OpenMP) ? Thanks Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
On Mon, 7 Jul 2014, Sylvestre Ledru wrote: Hello, On 17/06/2014 19:41, Joseph S. Myers wrote: On Tue, 17 Jun 2014, Sylvestre Ledru wrote: OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? That looks like the right things to test, if there are no changes for anything other than those options. Here it is: https://github.com/sylvestre/gcc/commit/db8aaac91aa09fd1ec1cc8974586aec45a221e71 Is that what you expected? The test Wmissing-return2.c only has one of the two warnings. But as per -Wreturn-type = Run both, and for backwards compatibility with the existing definition of -Wreturn-type, both warnings should appear for this test. (It's simply that only a subset of -Wreturn-type seems suitable to enable by default for C. Maybe the default subset, -Wreturn-type -Wno-missing-return (which is a combination that should also be included in the testcases), should have its own option name, although I don't know what that would be.) -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
Joseph, ping :) (I know you were in holidays) S On 07/07/2014 19:17, Sylvestre Ledru wrote: Hello, On 17/06/2014 19:41, Joseph S. Myers wrote: On Tue, 17 Jun 2014, Sylvestre Ledru wrote: OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? That looks like the right things to test, if there are no changes for anything other than those options. Here it is: https://github.com/sylvestre/gcc/commit/db8aaac91aa09fd1ec1cc8974586aec45a221e71 Is that what you expected? Besides that, are you OK with my changes? (with the tests updated) The tests are key to reviewing whether the code changes actually do the right thing. Right. Thanks again for your help and comments, it is really appreciated, Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
Hello, On 17/06/2014 19:41, Joseph S. Myers wrote: On Tue, 17 Jun 2014, Sylvestre Ledru wrote: OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? That looks like the right things to test, if there are no changes for anything other than those options. Here it is: https://github.com/sylvestre/gcc/commit/db8aaac91aa09fd1ec1cc8974586aec45a221e71 Is that what you expected? Besides that, are you OK with my changes? (with the tests updated) The tests are key to reviewing whether the code changes actually do the right thing. Right. Thanks again for your help and comments, it is really appreciated, Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
On 05/06/2014 20:01, Joseph S. Myers wrote:
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.)
OK.
As attachment, you will find a potential implementation. Is that what
you expect?
Also, at least one testsuite change in your patch is wrong.
OK. Thanks. I've probably made other (I update +1300 of them)
Thanks
Sylvestre
From 1b936c618c58dc0e899fa9f56013de48f7e4dcd6 Mon Sep 17 00:00:00 2001
From: Sylvestre Ledru sylves...@debian.org
Date: Tue, 17 Jun 2014 18:48:29 +0200
Subject: [PATCH 2/2] Enable Wimplicit 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 050d400..9b9ede7 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -460,7 +460,7 @@ C ObjC Var(warn_implicit_function_declaration) Init(-1) Warning LangEnabledBy(C
Warn about implicit function declarations
Wimplicit-int
-C ObjC Var(warn_implicit_int) Warning LangEnabledBy(C ObjC,Wimplicit)
+C ObjC Var(warn_implicit_int) Warning
Warn when a declaration does not specify a type
Wimport
--
2.0.0
From 80cd3dff34f74058ab66b69e0e01a05eaf686338 Mon Sep 17 00:00:00 2001
From: Sylvestre Ledru sylves...@debian.org
Date: Tue, 17 Jun 2014 18:48:12 +0200
Subject: [PATCH 1/2] Introduce -Wmissing-return (Was part of -Wreturn-type
which is now enabled by default)
---
gcc/c-family/c.opt| 4
gcc/doc/invoke.texi | 10 +-
gcc/fortran/options.c | 4
gcc/tree-cfg.c| 4 ++--
4 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 91f8275..050d400 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -697,6 +697,10 @@ Wreturn-type
C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn whenever a function's return type defaults to \int\ (C), or about inconsistent return types (C++)
+Wmissing-return
+C ObjC C++ ObjC++ Var(warn_missing_return) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn whenever control may reach end of non-void function
+
Wselector
ObjC ObjC++ Var(warn_selector) Warning
Warn if a selector has multiple methods
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 9a34f1c..9911e86 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -258,7 +258,7 @@ Objective-C and Objective-C++ Dialects}.
-Winvalid-pch -Wlarger-than=@var{len} -Wunsafe-loop-optimizations @gol
-Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
-Wmain -Wmaybe-uninitialized -Wmissing-braces -Wmissing-field-initializers @gol
--Wmissing-include-dirs @gol
+-Wmissing-include-dirs -Wmissing-return @gol
-Wno-multichar -Wnonnull -Wno-overflow -Wopenmp-simd @gol
-Woverlength-strings -Wpacked -Wpacked-bitfield-compat -Wpadded @gol
-Wparentheses -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
@@ -3327,6 +3327,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
-Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)} @gol
-Wmaybe-uninitialized @gol
-Wmissing-braces @r{(only for C/ObjC)} @gol
+-Wmissing-return @gol
-Wnonnull @gol
-Wopenmp-simd @gol
-Wparentheses @gol
@@ -3657,6 +3658,13 @@ the following example, the initializer for @samp{a} is not fully
bracketed, but that for @samp{b} is fully bracketed. This warning is
enabled by @option{-Wall} in C.
+@item -Wmissing-return
+@opindex Wmissing-return
+@opindex Wno-missing-return
+Warn whenever falling off the end of the function body (I.e. without
+any return).
+This warning is enabled by @option{-Wall} for C and C++.
+
@smallexample
int a[2][2] = @{ 0, 1, 2, 3 @};
int b[2][2] = @{ @{ 0, 1 @}, @{ 2, 3 @} @};
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index a2b91ca..fe71230 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -698,6 +698,10 @@ gfc_handle_option (size_t scode, const char *arg, int value,
gfc_option.warn_line_truncation = value;
break;
+case OPT_Wmissing_return:
+ warn_missing_return = value;
+ break;
+
case OPT_Wrealloc_lhs:
gfc_option.warn_realloc_lhs = value;
break;
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index e824619..2fd342e 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -8265,7 +8265,7 @@ pass_warn_function_return::execute (function *fun)
/* If we see return; in some basic block, then we do reach the end
without returning a value. */
- else if (warn_return_type
+ else if
Re: [Patch] PR55189 enable -Wreturn-type by default
On Tue, 17 Jun 2014, Sylvestre Ledru wrote: On 05/06/2014 20:01, Joseph S. Myers wrote: 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.) OK. As attachment, you will find a potential implementation. Is that what you expect? It would help a lot if it included testcases for what various options / option combinations do / do not enable. I expect that each option continues to enable the warnings it does at present (so if a user explicitly does -Wreturn-type it also enables the -Wmissing-return warnings, for example) - but some warnings would start to be enabled by default. If someone does e.g. -Wno-implicit that would disable the default -Wimplicit-int; if they do -Wno-implicit -Wimplicit that would have the same effect as just -Wimplicit (so keeping the default warnings enabled, and possibly enabling others). -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On 17/06/2014 19:15, Joseph S. Myers wrote: On Tue, 17 Jun 2014, Sylvestre Ledru wrote: On 05/06/2014 20:01, Joseph S. Myers wrote: 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.) OK. As attachment, you will find a potential implementation. Is that what you expect? It would help a lot if it included testcases for what various options / option combinations do / do not enable. OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? I expect that each option continues to enable the warnings it does at present (so if a user explicitly does -Wreturn-type it also enables the -Wmissing-return warnings, for example) - but some warnings would start to be enabled by default. If someone does e.g. -Wno-implicit that would disable the default -Wimplicit-int; if they do -Wno-implicit -Wimplicit that would have the same effect as just -Wimplicit (so keeping the default warnings enabled, and possibly enabling others). OK. I will try to implement that later (I don't think -Wimplicit-int is necessary to enable -Wreturn-type by default). Besides that, are you OK with my changes? (with the tests updated) Thanks, Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
On Tue, 17 Jun 2014, Sylvestre Ledru wrote: OK. I will do that. We should test the following: * default = run just -Wreturn-type * -Wreturn-type = Run both * -Wreturn-type + -Wmissing-return = Run both * -Wno-return-type + -Wmissing-return = Run just the second one * -Wno-return-type + -Wno-missing-return = Run none Do you see any other? That looks like the right things to test, if there are no changes for anything other than those options. Besides that, are you OK with my changes? (with the tests updated) The tests are key to reviewing whether the code changes actually do the right thing. -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type 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
Re: [Patch] PR55189 enable -Wreturn-type by default
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.)
--
Joseph S. Myers
jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On Jun 4, 2014, at 12:49 PM, Sylvestre Ledru sylves...@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.
Re: [Patch] PR55189 enable -Wreturn-type by default
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.) -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] PR55189 enable -Wreturn-type by default
On 01/16/2014 02:44 PM, Jason Merrill wrote:
To avoid spurious warnings on code with infinite loops we could add a
simple check for infinite loops and suppress the warning in that case.
Basically, if we see a loop with an always-true condition and no breaks.
Like so:
Tested x86_64-pc-linux-gnu, applying to trunk.
commit 9e1ee1aae068265870982c7b615c9e18136ab38a
Author: Jason Merrill ja...@redhat.com
Date: Thu Jan 16 16:39:58 2014 -0500
PR c++/55189
* cp-tree.h (struct language_function): Add infinite_loop and
infinite_loops.
(current_function_infinite_loop): New.
* semantics.c (begin_maybe_infinite_loop, end_maybe_infinite_loop)
(break_maybe_infinite_loop): New.
(finish_while_stmt_cond, finish_while_stmt, begin_do_stmt)
(finish_do_stmt, finish_for_cond, finish_for_stmt)
(begin_range_for_stmt): Use them.
* decl.c (finish_function): Don't warn about missing return
if current_function_infinite_loop.
* pt.c (instantiate_decl): Copy current_function_infinite_loop.
* parser.c (cp_parser_jump_statement): Call break_maybe_infinite_loop.
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 96af562f..ab75db8 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -1127,6 +1127,7 @@ struct GTY(()) language_function {
BOOL_BITFIELD returns_value : 1;
BOOL_BITFIELD returns_null : 1;
BOOL_BITFIELD returns_abnormally : 1;
+ BOOL_BITFIELD infinite_loop: 1;
BOOL_BITFIELD x_in_function_try_handler : 1;
BOOL_BITFIELD x_in_base_initializer : 1;
@@ -1136,6 +1137,9 @@ struct GTY(()) language_function {
htab_t GTY((param_is(struct named_label_entry))) x_named_labels;
cp_binding_level *bindings;
vectree, va_gc *x_local_names;
+ /* Tracking possibly infinite loops. This is a vectree only because
+ vecbool doesn't work with gtype. */
+ vectree, va_gc *infinite_loops;
htab_t GTY((param_is (struct cxx_int_tree_map))) extern_decl_map;
};
@@ -1194,6 +1198,12 @@ struct GTY(()) language_function {
#define current_function_returns_abnormally \
cp_function_chain-returns_abnormally
+/* Set to 0 at beginning of a function definition, set to 1 if we see an
+ obvious infinite loop. This can have false positives and false
+ negatives, so it should only be used as a heuristic. */
+
+#define current_function_infinite_loop cp_function_chain-infinite_loop
+
/* Nonzero if we are processing a base initializer. Zero elsewhere. */
#define in_base_initializer cp_function_chain-x_in_base_initializer
@@ -5671,6 +5681,7 @@ extern bool perform_or_defer_access_check (tree, tree, tree,
extern int stmts_are_full_exprs_p (void);
extern void init_cp_semantics (void);
extern tree do_poplevel(tree);
+extern void break_maybe_infinite_loop (void);
extern void add_decl_expr (tree);
extern tree maybe_cleanup_point_expr_void (tree);
extern tree finish_expr_stmt (tree);
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 5906653..bfe1daf 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -14000,6 +14000,8 @@ finish_function (int flags)
!current_function_returns_value !current_function_returns_null
/* Don't complain if we abort or throw. */
!current_function_returns_abnormally
+ /* Don't complain if there's an infinite loop. */
+ !current_function_infinite_loop
/* Don't complain if we are declared noreturn. */
!TREE_THIS_VOLATILE (fndecl)
!DECL_NAME (DECL_RESULT (fndecl))
@@ -14064,6 +14066,7 @@ finish_function (int flags)
f-x_return_value = NULL;
f-bindings = NULL;
f-extern_decl_map = NULL;
+ f-infinite_loops = NULL;
}
/* Clear out the bits we don't need. */
local_names = NULL;
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 3bc943b..9440df6 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -10636,6 +10636,8 @@ cp_parser_jump_statement (cp_parser* parser)
gcc_assert ((in_statement IN_SWITCH_STMT)
|| in_statement == IN_ITERATION_STMT);
statement = finish_break_stmt ();
+ if (in_statement == IN_ITERATION_STMT)
+ break_maybe_infinite_loop ();
break;
case IN_OMP_BLOCK:
error_at (token-location, invalid exit from OpenMP structured block);
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 2e7cf60..400a143 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -19670,6 +19670,10 @@ instantiate_decl (tree d, int defer_ok,
so that finish_function knows where we are. */
input_location
= DECL_STRUCT_FUNCTION (code_pattern)-function_end_locus;
+
+ /* Remember if we saw an infinite loop in the template. */
+ current_function_infinite_loop
+ = DECL_STRUCT_FUNCTION (code_pattern)-language-infinite_loop;
}
/* We don't need the local specializations any more. */
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index eb04266..d911cd5 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -486,6 +486,62 @@ push_cleanup (tree decl, tree cleanup, bool eh_only)
Re: [Patch] PR55189 enable -Wreturn-type by default
On 23/01/2014 10:48, Jason Merrill wrote: On 01/16/2014 02:44 PM, Jason Merrill wrote: To avoid spurious warnings on code with infinite loops we could add a simple check for infinite loops and suppress the warning in that case. Basically, if we see a loop with an always-true condition and no breaks. Like so: Tested x86_64-pc-linux-gnu, applying to trunk. Thanks for that! S
Re: [Patch] PR55189 enable -Wreturn-type by default
On 16/01/2014 11:44, Jason Merrill wrote: My preference would be to turn -Wreturn-type on by default, but not create the separate -Wmissing-return flag. As I argued in 2002, there should only be one flag. I don't have any opinion on the subject. The separate option or not is fine with me. I am just following an advice received :) Sylvestre
Re: [Patch] PR55189 enable -Wreturn-type by default
My preference would be to turn -Wreturn-type on by default, but not create the separate -Wmissing-return flag. As I argued in 2002, there should only be one flag. To avoid spurious warnings on code with infinite loops we could add a simple check for infinite loops and suppress the warning in that case. Basically, if we see a loop with an always-true condition and no breaks. Jason
Re: [Patch] PR55189 enable -Wreturn-type by default
2013/12/21 Sylvestre Ledru sylves...@debian.org: Hello Following this thread http://gcc.gnu.org/ml/gcc/2013-11/msg00260.html and this bug, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55189 I would like to propose the two following patches: I am activating -Wreturn-type by defaut and add the option -Wmissing-return [snip] Index: gcc/ChangeLog === --- gcc/ChangeLog (révision 206154) +++ gcc/ChangeLog (copie de travail) @@ -1,3 +1,11 @@ +2013-12-20 Sylvestre Ledru sylves...@debian.org + +PR target/55189 +* -Wreturn-type enabled by default. + * Introduce back the option -Wmissing-return (enabled by -Wall) + It was included by default with -Wreturn-type + * Update all tests failing because of these changes. + 2013-12-20 Eric Botcazou ebotca...@adacore.com * config/arm/arm.c (arm_expand_prologue): In a nested APCS frame with Hi, Sylvestre, Sorry I have no right to approve this patch. But I notice your ChangeLog formatting is not correct. You can refer to other entries in ChangeLog to refine yours, and then resubmit the patch for review. :) Best regards, jasonwucj
Re: [Patch] PR55189 enable -Wreturn-type by default
Chung-Wu wrote: But I notice your ChangeLog formatting is not correct. You can refer to other entries in ChangeLog to refine yours, and then resubmit the patch for review. :) Or - use contrib/mklog to autogenerate template ChangeLog for you. -Y
