diff mbox

Use append attribution to CFLAGS in wcsmbs

Message ID 1488991673-21932-1-git-send-email-gftg@linux.vnet.ibm.com
State New
Headers show

Commit Message

Gabriel F. T. Gomes March 8, 2017, 4:47 p.m. UTC
CFLAGS for several files in wcsmbs/Makefile are set without the append
operator ("+="), thus ignoring other attributions.  This patch changes
that so Makefiles in sysdeps can set extra compilation flags.  This is
being done in preparation for future float128 patches.

Tested for powerpc64le and x86_64.

2017-03-07  Gabriel F. T. Gomes  <gftg@linux.vnet.ibm.com>

	* wcsmbs/Makefile (CFLAGS-wcstol.c, CFLAGS-wcstoul.c)
	(CFLAGS-wcstoll.c, CFLAGS-wcstoull.c, CFLAGS-wcstod.c)
	(CFLAGS-wcstold.c, CFLAGS-wcstof.c, CFLAGS-wcstol_l.c)
	(CFLAGS-wcstoul_l.c, CFLAGS-wcstoll_l.c, CFLAGS-wcstoull_l.c)
	(CFLAGS-wcstod_l.c, CFLAGS-wcstold_l.c, CFLAGS-wcstof_l.c)
	(CPPFLAGS-tst-wchar-h.c): Use the append operator.
---
 wcsmbs/Makefile | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Mike Frysinger March 12, 2017, 2:04 a.m. UTC | #1
On 08 Mar 2017 13:47, Gabriel F. T. Gomes wrote:
> CFLAGS for several files in wcsmbs/Makefile are set without the append
> operator ("+="), thus ignoring other attributions.  This patch changes
> that so Makefiles in sysdeps can set extra compilation flags.  This is
> being done in preparation for future float128 patches.

i'm not against this, but it seems like we should be doing this
everywhere ?  when i grep for CFLAGS-xxx.c, there's about 650
using = and 250 using +=.

even in this file, you've only changed one block ... there's
more entries in it still using =.
-mike
Joseph Myers March 13, 2017, 4:37 p.m. UTC | #2
On Sat, 11 Mar 2017, Mike Frysinger wrote:

> On 08 Mar 2017 13:47, Gabriel F. T. Gomes wrote:
> > CFLAGS for several files in wcsmbs/Makefile are set without the append
> > operator ("+="), thus ignoring other attributions.  This patch changes
> > that so Makefiles in sysdeps can set extra compilation flags.  This is
> > being done in preparation for future float128 patches.
> 
> i'm not against this, but it seems like we should be doing this
> everywhere ?  when i grep for CFLAGS-xxx.c, there's about 650
> using = and 250 using +=.

See the discussion starting at 
<https://sourceware.org/ml/libc-alpha/2012-11/msg00798.html> (and 
<https://sourceware.org/ml/libc-alpha/2013-01/msg00247.html>).  Given that 
CFLAGS are the right way of handling a particular file (rather than 
diagnostic pragmas, #define, etc., directly in the source code), I think 
appending makes sense unless there is a specific reason for not doing so 
in a particular case (and working out if any existing cases are 
deliberately not appending may be tricky).
Gabriel F. T. Gomes March 13, 2017, 7:03 p.m. UTC | #3
On Mon, 13 Mar 2017 16:37:43 +0000
Joseph Myers <joseph@codesourcery.com> wrote:

> On Sat, 11 Mar 2017, Mike Frysinger wrote:
> 
> > On 08 Mar 2017 13:47, Gabriel F. T. Gomes wrote:  
> > > CFLAGS for several files in wcsmbs/Makefile are set without the append
> > > operator ("+="), thus ignoring other attributions.  This patch changes
> > > that so Makefiles in sysdeps can set extra compilation flags.  This is
> > > being done in preparation for future float128 patches.  
> > 
> > i'm not against this, but it seems like we should be doing this
> > everywhere ?  when i grep for CFLAGS-xxx.c, there's about 650
> > using = and 250 using +=.  
> 
> See the discussion starting at 
> <https://sourceware.org/ml/libc-alpha/2012-11/msg00798.html> (and 
> <https://sourceware.org/ml/libc-alpha/2013-01/msg00247.html>).  Given that 
> CFLAGS are the right way of handling a particular file (rather than 
> diagnostic pragmas, #define, etc., directly in the source code), I think 
> appending makes sense unless there is a specific reason for not doing so 
> in a particular case (and working out if any existing cases are 
> deliberately not appending may be tricky).
> 

In the discussion, there seems to be some consensus as to using
sysdep-CFLAGS-<filename> (and sysdep-CFLAGS += $(sysdep-CFLAGS-$(<F))),
instead of CFLAGS (as mentioned in the third item of [1]).  As well
as there seems to be consensus on using the append operator (as
mentioned in [2])

(Please, let me know if my perception of consensus is wrong here)

[1] https://sourceware.org/ml/libc-alpha/2013-01/msg00337.html
[2] https://sourceware.org/ml/libc-alpha/2013-01/msg00338.html

I wasn't aware of sysdep-CFLAGS when I sent this patch.

With the use of sysdep-CFLAGS, there is no need to change the =
operator to += in wcsmbs/Makefile.

So, may I drop this patch, or is it still useful?
Mike Frysinger March 13, 2017, 9:24 p.m. UTC | #4
On 13 Mar 2017 16:03, Gabriel F. T. Gomes wrote:
> On Mon, 13 Mar 2017 16:37:43 +0000 Joseph Myers wrote:
> > On Sat, 11 Mar 2017, Mike Frysinger wrote:
> > > On 08 Mar 2017 13:47, Gabriel F. T. Gomes wrote:  
> > > > CFLAGS for several files in wcsmbs/Makefile are set without the append
> > > > operator ("+="), thus ignoring other attributions.  This patch changes
> > > > that so Makefiles in sysdeps can set extra compilation flags.  This is
> > > > being done in preparation for future float128 patches.  
> > > 
> > > i'm not against this, but it seems like we should be doing this
> > > everywhere ?  when i grep for CFLAGS-xxx.c, there's about 650
> > > using = and 250 using +=.  
> > 
> > See the discussion starting at 
> > <https://sourceware.org/ml/libc-alpha/2012-11/msg00798.html> (and 
> > <https://sourceware.org/ml/libc-alpha/2013-01/msg00247.html>).  Given that 
> > CFLAGS are the right way of handling a particular file (rather than 
> > diagnostic pragmas, #define, etc., directly in the source code), I think 
> > appending makes sense unless there is a specific reason for not doing so 
> > in a particular case (and working out if any existing cases are 
> > deliberately not appending may be tricky).
> > 
> 
> In the discussion, there seems to be some consensus as to using
> sysdep-CFLAGS-<filename> (and sysdep-CFLAGS += $(sysdep-CFLAGS-$(<F))),
> instead of CFLAGS (as mentioned in the third item of [1]).  As well
> as there seems to be consensus on using the append operator (as
> mentioned in [2])
> 
> (Please, let me know if my perception of consensus is wrong here)

rereading the threads again, it looks like we're all in agreement.
let's land a commit to change all of the "=" to "+=", and let's add
support for $(sysdep-CFLAGS-$(<F)).
-mike
diff mbox

Patch

diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
index d6b214b..b29c85e 100644
--- a/wcsmbs/Makefile
+++ b/wcsmbs/Makefile
@@ -75,21 +75,21 @@  CFLAGS-wcwidth.c = -I../wctype
 CFLAGS-wcswidth.c = -I../wctype
 
 strtox-CFLAGS = -I../include
-CFLAGS-wcstol.c = $(strtox-CFLAGS)
-CFLAGS-wcstoul.c = $(strtox-CFLAGS)
-CFLAGS-wcstoll.c = $(strtox-CFLAGS)
-CFLAGS-wcstoull.c = $(strtox-CFLAGS)
-CFLAGS-wcstod.c = $(strtox-CFLAGS)
-CFLAGS-wcstold.c = $(strtox-CFLAGS)
-CFLAGS-wcstof.c = $(strtox-CFLAGS)
-CFLAGS-wcstol_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstoul_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstoll_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstoull_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstod_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstold_l.c = $(strtox-CFLAGS)
-CFLAGS-wcstof_l.c = $(strtox-CFLAGS)
-CPPFLAGS-tst-wchar-h.c = -D_FORTIFY_SOURCE=2
+CFLAGS-wcstol.c += $(strtox-CFLAGS)
+CFLAGS-wcstoul.c += $(strtox-CFLAGS)
+CFLAGS-wcstoll.c += $(strtox-CFLAGS)
+CFLAGS-wcstoull.c += $(strtox-CFLAGS)
+CFLAGS-wcstod.c += $(strtox-CFLAGS)
+CFLAGS-wcstold.c += $(strtox-CFLAGS)
+CFLAGS-wcstof.c += $(strtox-CFLAGS)
+CFLAGS-wcstol_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstoul_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstoll_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstoull_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstod_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstold_l.c += $(strtox-CFLAGS)
+CFLAGS-wcstof_l.c += $(strtox-CFLAGS)
+CPPFLAGS-tst-wchar-h.c += -D_FORTIFY_SOURCE=2
 
 CFLAGS-isoc99_wscanf.c += -fexceptions
 CFLAGS-isoc99_fwscanf.c += -fexceptions