Message ID | 1488991673-21932-1-git-send-email-gftg@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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
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).
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?
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 --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