diff mbox series

configure: Remove --enable-all-warnings option

Message ID 20230724172227.2789361-1-adhemerval.zanella@linaro.org
State New
Headers show
Series configure: Remove --enable-all-warnings option | expand

Commit Message

Adhemerval Zanella Netto July 24, 2023, 5:22 p.m. UTC 
The option is not activelly tested and has bitrotten, to fix it
would require a lot of work and multiple fixes.  A better option
would to evaluate each option and enable the warning if it makes
sense.
---
 Makeconfig     |  7 +------
 config.make.in |  1 -
 configure      | 11 -----------
 configure.ac   | 10 ----------
 4 files changed, 1 insertion(+), 28 deletions(-)

Comments

Siddhesh Poyarekar July 24, 2023, 5:44 p.m. UTC | #1 
On 2023-07-24 13:22, Adhemerval Zanella via Libc-alpha wrote:
> The option is not activelly tested and has bitrotten, to fix it
> would require a lot of work and multiple fixes.  A better option
> would to evaluate each option and enable the warning if it makes
> sense.

Can we do that evaluation and then drop this flag?  I feel like a number 
of them could be added in the default configuration.

Thanks,
Sid

> ---
>   Makeconfig     |  7 +------
>   config.make.in |  1 -
>   configure      | 11 -----------
>   configure.ac   | 10 ----------
>   4 files changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/Makeconfig b/Makeconfig
> index 77d7fd14df..c4dd9ea8f2 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -857,12 +857,7 @@ host-test-program-cmd = $(host-built-program-cmd)
>   endif
>   
>   # Extra flags to pass to GCC.
> -ifeq ($(all-warnings),yes)
> -+gccwarn := -Wall -Wwrite-strings -Wcast-qual -Wbad-function-cast -Wmissing-noreturn -Wmissing-prototypes -Wmissing-declarations -Wcomment -Wcomments -Wtrigraphs -Wsign-compare -Wfloat-equal -Wmultichar
> -else
> -+gccwarn := -Wall -Wwrite-strings
> -endif
> -+gccwarn += -Wundef
> +gccwarn := -Wall -Wwrite-strings -Wundef
>   ifeq ($(enable-werror),yes)
>   +gccwarn += -Werror
>   endif
> diff --git a/config.make.in b/config.make.in
> index d487a4f4e9..0c4baa2731 100644
> --- a/config.make.in
> +++ b/config.make.in
> @@ -50,7 +50,6 @@ c++-sysincludes = @CXX_SYSINCLUDES@
>   c++-cstdlib-header = @CXX_CSTDLIB_HEADER@
>   c++-cmath-header = @CXX_CMATH_HEADER@
>   c++-bits-std_abs-h = @CXX_BITS_STD_ABS_H@
> -all-warnings = @all_warnings@
>   enable-werror = @enable_werror@
>   
>   have-z-execstack = @libc_cv_z_execstack@
> diff --git a/configure b/configure
> index 4ef387146d..eeb3ef49b8 100755
> --- a/configure
> +++ b/configure
> @@ -705,7 +705,6 @@ libc_cv_nss_crypt
>   build_crypt
>   memory_tagging
>   enable_werror
> -all_warnings
>   force_install
>   bindnow
>   hardcoded_path_in_tests
> @@ -804,7 +803,6 @@ enable_static_nss
>   enable_force_install
>   enable_maintainer_mode
>   enable_kernel
> -enable_all_warnings
>   enable_werror
>   enable_multi_arch
>   enable_memory_tagging
> @@ -1478,7 +1476,6 @@ Optional Features:
>                             sometimes confusing) to the casual installer
>     --enable-kernel=VERSION compile for compatibility with kernel not older than
>                             VERSION
> -  --enable-all-warnings   enable all useful warnings gcc can issue
>     --disable-werror        do not build with -Werror
>     --enable-multi-arch     enable single DSO with optimizations for multiple
>                             architectures
> @@ -4526,14 +4523,6 @@ else
>     fi
>   fi
>   
> -# Check whether --enable-all-warnings was given.
> -if test ${enable_all_warnings+y}
> -then :
> -  enableval=$enable_all_warnings; all_warnings=$enableval
> -fi
> -
> -
> -
>   # Check whether --enable-werror was given.
>   if test ${enable_werror+y}
>   then :
> diff --git a/configure.ac b/configure.ac
> index 12d1f50b14..6601331a06 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -277,16 +277,6 @@ else
>     fi
>   fi
>   
> -dnl For the development we sometimes want gcc to issue even more warnings.
> -dnl This is not the default since many of the extra warnings are not
> -dnl appropriate.
> -AC_ARG_ENABLE([all-warnings],
> -	      AS_HELP_STRING([--enable-all-warnings],
> -			     [enable all useful warnings gcc can issue]),
> -	      [all_warnings=$enableval],
> -	      [])
> -AC_SUBST(all_warnings)
> -
>   AC_ARG_ENABLE([werror],
>   	      AS_HELP_STRING([--disable-werror],
>   			     [do not build with -Werror]),
Adhemerval Zanella Netto July 26, 2023, 1:01 p.m. UTC | #2 
On 24/07/23 14:44, Siddhesh Poyarekar wrote:
> On 2023-07-24 13:22, Adhemerval Zanella via Libc-alpha wrote:
>> The option is not activelly tested and has bitrotten, to fix it
>> would require a lot of work and multiple fixes.  A better option
>> would to evaluate each option and enable the warning if it makes
>> sense.
> 
> Can we do that evaluation and then drop this flag?  I feel like a number of them could be added in the default configuration.

I have evaluate some of them, but some are really time consuming (like
Wcast-qual), and others are required in some code (like -Wbad-function-cast).
Some are really not that useful, like -Wcomment/-Wcomments/-Wtrigraphs;
but I think  -Wmissing-prototypes/-Wmissing-declarations might be useful.
I would like to add -Wtrampolines, but it would require to maskoff for
hurd.

I started to work on enable -Wmissing-noreturn, but I did not see much
gain in code generation and it was a quite large patch.

In any case, I still think we should remove the configure regardless
since it is currently unusable. 

> 
> Thanks,
> Sid
> 
>> ---
>>   Makeconfig     |  7 +------
>>   config.make.in |  1 -
>>   configure      | 11 -----------
>>   configure.ac   | 10 ----------
>>   4 files changed, 1 insertion(+), 28 deletions(-)
>>
>> diff --git a/Makeconfig b/Makeconfig
>> index 77d7fd14df..c4dd9ea8f2 100644
>> --- a/Makeconfig
>> +++ b/Makeconfig
>> @@ -857,12 +857,7 @@ host-test-program-cmd = $(host-built-program-cmd)
>>   endif
>>     # Extra flags to pass to GCC.
>> -ifeq ($(all-warnings),yes)
>> -+gccwarn := -Wall -Wwrite-strings -Wcast-qual -Wbad-function-cast -Wmissing-noreturn -Wmissing-prototypes -Wmissing-declarations -Wcomment -Wcomments -Wtrigraphs -Wsign-compare -Wfloat-equal -Wmultichar
>> -else
>> -+gccwarn := -Wall -Wwrite-strings
>> -endif
>> -+gccwarn += -Wundef
>> +gccwarn := -Wall -Wwrite-strings -Wundef
>>   ifeq ($(enable-werror),yes)
>>   +gccwarn += -Werror
>>   endif
>> diff --git a/config.make.in b/config.make.in
>> index d487a4f4e9..0c4baa2731 100644
>> --- a/config.make.in
>> +++ b/config.make.in
>> @@ -50,7 +50,6 @@ c++-sysincludes = @CXX_SYSINCLUDES@
>>   c++-cstdlib-header = @CXX_CSTDLIB_HEADER@
>>   c++-cmath-header = @CXX_CMATH_HEADER@
>>   c++-bits-std_abs-h = @CXX_BITS_STD_ABS_H@
>> -all-warnings = @all_warnings@
>>   enable-werror = @enable_werror@
>>     have-z-execstack = @libc_cv_z_execstack@
>> diff --git a/configure b/configure
>> index 4ef387146d..eeb3ef49b8 100755
>> --- a/configure
>> +++ b/configure
>> @@ -705,7 +705,6 @@ libc_cv_nss_crypt
>>   build_crypt
>>   memory_tagging
>>   enable_werror
>> -all_warnings
>>   force_install
>>   bindnow
>>   hardcoded_path_in_tests
>> @@ -804,7 +803,6 @@ enable_static_nss
>>   enable_force_install
>>   enable_maintainer_mode
>>   enable_kernel
>> -enable_all_warnings
>>   enable_werror
>>   enable_multi_arch
>>   enable_memory_tagging
>> @@ -1478,7 +1476,6 @@ Optional Features:
>>                             sometimes confusing) to the casual installer
>>     --enable-kernel=VERSION compile for compatibility with kernel not older than
>>                             VERSION
>> -  --enable-all-warnings   enable all useful warnings gcc can issue
>>     --disable-werror        do not build with -Werror
>>     --enable-multi-arch     enable single DSO with optimizations for multiple
>>                             architectures
>> @@ -4526,14 +4523,6 @@ else
>>     fi
>>   fi
>>   -# Check whether --enable-all-warnings was given.
>> -if test ${enable_all_warnings+y}
>> -then :
>> -  enableval=$enable_all_warnings; all_warnings=$enableval
>> -fi
>> -
>> -
>> -
>>   # Check whether --enable-werror was given.
>>   if test ${enable_werror+y}
>>   then :
>> diff --git a/configure.ac b/configure.ac
>> index 12d1f50b14..6601331a06 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -277,16 +277,6 @@ else
>>     fi
>>   fi
>>   -dnl For the development we sometimes want gcc to issue even more warnings.
>> -dnl This is not the default since many of the extra warnings are not
>> -dnl appropriate.
>> -AC_ARG_ENABLE([all-warnings],
>> -          AS_HELP_STRING([--enable-all-warnings],
>> -                 [enable all useful warnings gcc can issue]),
>> -          [all_warnings=$enableval],
>> -          [])
>> -AC_SUBST(all_warnings)
>> -
>>   AC_ARG_ENABLE([werror],
>>             AS_HELP_STRING([--disable-werror],
>>                    [do not build with -Werror]),
Siddhesh Poyarekar July 26, 2023, 1:33 p.m. UTC | #3 
On 2023-07-26 09:01, Adhemerval Zanella Netto wrote:
> 
> 
> On 24/07/23 14:44, Siddhesh Poyarekar wrote:
>> On 2023-07-24 13:22, Adhemerval Zanella via Libc-alpha wrote:
>>> The option is not activelly tested and has bitrotten, to fix it
>>> would require a lot of work and multiple fixes.  A better option
>>> would to evaluate each option and enable the warning if it makes
>>> sense.
>>
>> Can we do that evaluation and then drop this flag?  I feel like a number of them could be added in the default configuration.
> 
> I have evaluate some of them, but some are really time consuming (like
> Wcast-qual), and others are required in some code (like -Wbad-function-cast).
> Some are really not that useful, like -Wcomment/-Wcomments/-Wtrigraphs;
> but I think  -Wmissing-prototypes/-Wmissing-declarations might be useful.
> I would like to add -Wtrampolines, but it would require to maskoff for
> hurd.
> 
> I started to work on enable -Wmissing-noreturn, but I did not see much
> gain in code generation and it was a quite large patch.
> 
> In any case, I still think we should remove the configure regardless
> since it is currently unusable.

OK, I don't really want to block on this because I see the point of the 
cleanups you're making.  How about filing a bug then, listing the 
warnings in --enable-all-warnings, stating that we need to evaluate them 
for inclusion into our default warnings?  That way we simplify the 
configure and also keep track of the flags that we need to eventually 
try and incorporate.

So LGTM for 2.39 with the bug filed to track future evaluation of the 
flags.  Hopefully if anybody is actually using this flag, they'll 
respond in the ~week it'll take for the freeze to thaw.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Thanks,
Sid
diff mbox series

Patch

diff --git a/Makeconfig b/Makeconfig
index 77d7fd14df..c4dd9ea8f2 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -857,12 +857,7 @@  host-test-program-cmd = $(host-built-program-cmd)
 endif
 
 # Extra flags to pass to GCC.
-ifeq ($(all-warnings),yes)
-+gccwarn := -Wall -Wwrite-strings -Wcast-qual -Wbad-function-cast -Wmissing-noreturn -Wmissing-prototypes -Wmissing-declarations -Wcomment -Wcomments -Wtrigraphs -Wsign-compare -Wfloat-equal -Wmultichar
-else
-+gccwarn := -Wall -Wwrite-strings
-endif
-+gccwarn += -Wundef
+gccwarn := -Wall -Wwrite-strings -Wundef
 ifeq ($(enable-werror),yes)
 +gccwarn += -Werror
 endif
diff --git a/config.make.in b/config.make.in
index d487a4f4e9..0c4baa2731 100644
--- a/config.make.in
+++ b/config.make.in
@@ -50,7 +50,6 @@  c++-sysincludes = @CXX_SYSINCLUDES@
 c++-cstdlib-header = @CXX_CSTDLIB_HEADER@
 c++-cmath-header = @CXX_CMATH_HEADER@
 c++-bits-std_abs-h = @CXX_BITS_STD_ABS_H@
-all-warnings = @all_warnings@
 enable-werror = @enable_werror@
 
 have-z-execstack = @libc_cv_z_execstack@
diff --git a/configure b/configure
index 4ef387146d..eeb3ef49b8 100755
--- a/configure
+++ b/configure
@@ -705,7 +705,6 @@  libc_cv_nss_crypt
 build_crypt
 memory_tagging
 enable_werror
-all_warnings
 force_install
 bindnow
 hardcoded_path_in_tests
@@ -804,7 +803,6 @@  enable_static_nss
 enable_force_install
 enable_maintainer_mode
 enable_kernel
-enable_all_warnings
 enable_werror
 enable_multi_arch
 enable_memory_tagging
@@ -1478,7 +1476,6 @@  Optional Features:
                           sometimes confusing) to the casual installer
   --enable-kernel=VERSION compile for compatibility with kernel not older than
                           VERSION
-  --enable-all-warnings   enable all useful warnings gcc can issue
   --disable-werror        do not build with -Werror
   --enable-multi-arch     enable single DSO with optimizations for multiple
                           architectures
@@ -4526,14 +4523,6 @@  else
   fi
 fi
 
-# Check whether --enable-all-warnings was given.
-if test ${enable_all_warnings+y}
-then :
-  enableval=$enable_all_warnings; all_warnings=$enableval
-fi
-
-
-
 # Check whether --enable-werror was given.
 if test ${enable_werror+y}
 then :
diff --git a/configure.ac b/configure.ac
index 12d1f50b14..6601331a06 100644
--- a/configure.ac
+++ b/configure.ac
@@ -277,16 +277,6 @@  else
   fi
 fi
 
-dnl For the development we sometimes want gcc to issue even more warnings.
-dnl This is not the default since many of the extra warnings are not
-dnl appropriate.
-AC_ARG_ENABLE([all-warnings],
-	      AS_HELP_STRING([--enable-all-warnings],
-			     [enable all useful warnings gcc can issue]),
-	      [all_warnings=$enableval],
-	      [])
-AC_SUBST(all_warnings)
-
 AC_ARG_ENABLE([werror],
 	      AS_HELP_STRING([--disable-werror],
 			     [do not build with -Werror]),