[libvirt] [PATCH] cfg.mk: Introduce rule for setlocale()
In the past we had some issues where setlocale() was called without corresponding include of locale.h. While on some systems this may work, on others the compilation failed. We should have a syntax-check rule for that to prevent this from happening again. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cfg.mk b/cfg.mk index 9e8fcec..36395c6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -929,6 +929,15 @@ sc_prohibit_mixed_case_abbreviations: halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \ $(_sc_search_regexp) +# Require #include <locale.h> in all files that call setlocale() +sc_require_locale_h: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + if test -z "$$(grep setlocale $$i)" ; then continue; fi; \ + if test -z "$$(grep 'include <locale.h>' $$i)" ; then \ + echo '$(ME): missing locale.h include in' $$i 1>&2; exit 1; \ + fi; \ + done; + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 2.0.0
On Wed, Jun 04, 2014 at 10:59:08AM +0200, Michal Privoznik wrote:
In the past we had some issues where setlocale() was called without corresponding include of locale.h. While on some systems this may work, on others the compilation failed. We should have a syntax-check rule for that to prevent this from happening again.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 9e8fcec..36395c6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -929,6 +929,15 @@ sc_prohibit_mixed_case_abbreviations: halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \ $(_sc_search_regexp)
+# Require #include <locale.h> in all files that call setlocale() +sc_require_locale_h: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + if test -z "$$(grep setlocale $$i)" ; then continue; fi; \
Why not: if ! grep -q setlocale $$i; then continue; fi ? It shouldn't be any bashism and I think it works everywhere. I just tried it in dash and it works. And if I may nitpick, you can also s/setlocale/\bsetlocale(/.
+ if test -z "$$(grep 'include <locale.h>' $$i)" ; then \
The same here. '# *include...' for the maximum bonus ;)
+ echo '$(ME): missing locale.h include in' $$i 1>&2; exit 1; \ + fi; \ + done; + # We don't use this feature of maint.mk. prev_version_file = /dev/null
ACK with the changes mentioned. Martin
On 06/04/2014 03:15 AM, Martin Kletzander wrote:
On Wed, Jun 04, 2014 at 10:59:08AM +0200, Michal Privoznik wrote:
In the past we had some issues where setlocale() was called without corresponding include of locale.h. While on some systems this may work, on others the compilation failed. We should have a syntax-check rule for that to prevent this from happening again.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 9 +++++++++ 1 file changed, 9 insertions(+)
+# Require #include <locale.h> in all files that call setlocale() +sc_require_locale_h: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + if test -z "$$(grep setlocale $$i)" ; then continue; fi; \
Why not:
if ! grep -q setlocale $$i; then continue; fi
Even simpler, let maint.mk do it for you: sc_require_locale_h: @require='include.*locale\.h' \ containing='setlocale *(' \ halt='setlocale() requires <locale.h>' \ $(_sc_search_regexp) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04.06.2014 18:21, Eric Blake wrote:
On 06/04/2014 03:15 AM, Martin Kletzander wrote:
On Wed, Jun 04, 2014 at 10:59:08AM +0200, Michal Privoznik wrote:
In the past we had some issues where setlocale() was called without corresponding include of locale.h. While on some systems this may work, on others the compilation failed. We should have a syntax-check rule for that to prevent this from happening again.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 9 +++++++++ 1 file changed, 9 insertions(+)
+# Require #include <locale.h> in all files that call setlocale() +sc_require_locale_h: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + if test -z "$$(grep setlocale $$i)" ; then continue; fi; \
Why not:
if ! grep -q setlocale $$i; then continue; fi
Even simpler, let maint.mk do it for you:
sc_require_locale_h: @require='include.*locale\.h' \ containing='setlocale *(' \ halt='setlocale() requires <locale.h>' \ $(_sc_search_regexp)
This is the maint.mk-ism I was looking for. Feel free to replace my code. Just out of pure curiosity - is your code any faster? Michal
On 06/04/2014 10:24 AM, Michal Privoznik wrote:
+# Require #include <locale.h> in all files that call setlocale() +sc_require_locale_h: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + if test -z "$$(grep setlocale $$i)" ; then continue; fi; \
Why not:
if ! grep -q setlocale $$i; then continue; fi
Even simpler, let maint.mk do it for you:
sc_require_locale_h: @require='include.*locale\.h' \ containing='setlocale *(' \ halt='setlocale() requires <locale.h>' \ $(_sc_search_regexp)
This is the maint.mk-ism I was looking for. Feel free to replace my code. Just out of pure curiosity - is your code any faster?
Yes. Your code spawns 1 or 2 greps per file (one spawn per iteration of the forloop); while maint.mk's rule does tricks like: : Filter by content; \ test -n "$$files" && test -n "$$containing" \ && { files=$$(grep -l "$$containing" $$files); } || :; \ that use a single grep process to drastically filter the set of files that then need to be tested. I see you already pushed, so I'll take your message as the ACK to go ahead and push my replacement. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Martin Kletzander -
Michal Privoznik