| Message ID | 1482833331-2261-1-git-send-email-mengdong.lin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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