[PATCH 1/2] ucm: Enable all warnings for automake C preprocessor

[PATCH 1/2] ucm: Enable all warnings for automake C preprocessor

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

To display warnings for us to fix them, add '-Wall' to AM_CPPFLAGS.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/ucm/Makefile.am b/src/ucm/Makefile.am
index 9d66b24..79e57d2 100644
--- a/src/ucm/Makefile.am
+++ b/src/ucm/Makefile.am
@@ -7,4 +7,4 @@ noinst_HEADERS = ucm_local.h
 all: libucm.la
 
 
-AM_CPPFLAGS=-I$(top_srcdir)/include
+AM_CPPFLAGS=-I$(top_srcdir)/include -Wall
-- 
2.7.4

Re: [PATCH 1/2] ucm: Enable all warnings for automake C preprocessor

[Takashi Sakamoto]

Hi,

On Dec 27 2016 19:08, mengdong.lin@linux.intel.com wrote:
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>
> To display warnings for us to fix them, add '-Wall' to AM_CPPFLAGS.
>
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>
> diff --git a/src/ucm/Makefile.am b/src/ucm/Makefile.am
> index 9d66b24..79e57d2 100644
> --- a/src/ucm/Makefile.am
> +++ b/src/ucm/Makefile.am
> @@ -7,4 +7,4 @@ noinst_HEADERS = ucm_local.h
>  all: libucm.la
>
>
> -AM_CPPFLAGS=-I$(top_srcdir)/include
> +AM_CPPFLAGS=-I$(top_srcdir)/include -Wall

As long as I know, the '-Wall' option is for C/C++ compiler. Therefore, 
it's not necessarily correct to add the option to 'AM_CPPFLAGS' as 
options for preprocessor. In a manual of GNU Automake, there're some 
samples for this option; '-I' and '-D'[0]. Usage of 'AM_CFLAGS' might be 
better in this case.

...But this indication is a bit nitpicking and I'm not so strongly 
oppose this patch. I'll manage to describe the reason.

In a manual of GNU compiler collection, there're samples of implicit 
rules for C/C++ compilers:
  * $(CC) $(CPPFLAGS) $(CFLAGS) -c
  * $(CXX) $(CPPFLAGS) $(CXXFLAGS) -c

If the place of 'CPPFLAGS' is replaced with options in AM_CPPFLAGS, your 
patch has an effect for both of C code and C++ code. This is somehow 
expected for some developers, while it's also be obtrusive because of 
sub-effect to C++ code in our case.

Currently, alsa-lib includes no C++ codes, while this patch could bring 
the obtrusiveness for future when some C++ codes were added (i.e. by 
code refactoring). But the result were preferable by itself, I believe.

Well, if Makefile.am includes 'mumble_CFLAGS' lines, options in 
'AM_CFLAGS' is ignored to generate the 'mumble'. For example, a command 
to generate 'user_ctl_element_set' object in 'test' directory is this 
case (I wrote it[2]).

Currently I cannot judge which way is better... I'm sorry but could I 
request your opinion for this patch with the view of these specifications?

[0]: 
https://www.gnu.org/software/automake/manual/html_node/Program-Variables.html
[1]: 
https://www.gnu.org/software/make/manual/html_node/Catalogue-of-Rules.html#Catalogue-of-Rules
[2]: 
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=test/Makefile.am;h=5f35159af2d64c12e5d394cff884765808d38e5b;hb=HEAD#l29


Regards

Takashi Sakamoto

Re: [PATCH 1/2] ucm: Enable all warnings for automake C preprocessor

[Takashi Iwai]

On Tue, 27 Dec 2016 11:08:51 +0100,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> To display warnings for us to fix them, add '-Wall' to AM_CPPFLAGS.

As Sakamoto-san already pointed, AM_CPPFLAGS is no correct variable to
set such a flag.

And, yet, an open question is whether we should add this
unconditionally or not.  In general, -W is a compiler-specific option,
and it's not always available.  That's why we didn't set it usually,
but leaving it in extra $CFLAGS pass mechanism at the invocation time,
as found in gitcompile script.

Of course, all gcc versions have -W options, but a 3rd party compiler
might not.  We may cover via autoconf, but it'll be messy too.  So, if
we really want to be safe, we shouldn't set it in Makefile.am but
leave user decide.

OTOH, -Wall is set in some subdirectories, and it's fine in 99.999%
cases.  We can add it mostly safely.

That said, I'm not quite sure (or don't care much) about this.  But if
we add -Wall, it'd be better to handle it in the top level, instead of
each subdirectory.


thanks,

Takashi

> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> diff --git a/src/ucm/Makefile.am b/src/ucm/Makefile.am
> index 9d66b24..79e57d2 100644
> --- a/src/ucm/Makefile.am
> +++ b/src/ucm/Makefile.am
> @@ -7,4 +7,4 @@ noinst_HEADERS = ucm_local.h
>  all: libucm.la
>  
>  
> -AM_CPPFLAGS=-I$(top_srcdir)/include
> +AM_CPPFLAGS=-I$(top_srcdir)/include -Wall
> -- 
> 2.7.4
>