Re: [PATCH] regex: fix a SIZE_MAX macro redefinition warning

[Junio C Hamano]

Ramsay Jones  writes:

> The original version of this patch looked like this:
> ...
> So, just move the unconditional inclusion to the start of the compilation
> unit root file, before the #include of the regex_internal.h header.
>
> In some ways this is a better fix, because it makes it clear that, currently,
> the compat/regex code requires . This would remove the need for
> such a comment.

Sounds sensible to me too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regex: fix a SIZE_MAX macro redefinition warning

[Johannes Schindelin]

Hi Ramsay,

On Sun, 5 Jun 2016, Ramsay Jones wrote:

> > Other than that, I think this patch is an improvement.
> 
> Thanks. What do you think of replacing it with the original patch
> (above)?

That would work for me, too.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regex: fix a SIZE_MAX macro redefinition warning

[Ramsay Jones]

On 05/06/16 08:15, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> thanks for working on this!
> 
> On Sat, 4 Jun 2016, Ramsay Jones wrote:
> 
>> diff --git a/Makefile b/Makefile
>> index 0d59718..3f6c70a 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1987,7 +1987,7 @@ endif
>>  
>>  ifdef NO_REGEX
>>  compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>> --DGAWK -DNO_MBSUPPORT
>> +-DGAWK -DNO_MBSUPPORT -DHAVE_STDINT_H
> 
> Maybe a comment here, something like "the fallback regex implementation
> *requires* stdint.h"?

The original version of this patch looked like this:

  diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
  index fba5986..d8bde06 100644
  --- a/compat/regex/regcomp.c
  +++ b/compat/regex/regcomp.c
  @@ -18,8 +18,6 @@
  Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
  02110-1301 USA.  */
   
  -#include 
  -
   static reg_errcode_t re_compile_internal (regex_t *preg, const char * 
pattern,
  size_t length, reg_syntax_t syntax);
   static void re_compile_fastmap_iter (regex_t *bufp,
  diff --git a/compat/regex/regex.c b/compat/regex/regex.c
  index 6aaae00..5cb23e5 100644
  --- a/compat/regex/regex.c
  +++ b/compat/regex/regex.c
  @@ -60,6 +60,7 @@
  GNU regex allows.  Include it before , which correctly
  #undefs RE_DUP_MAX and sets it to the right value.  */
   #include 
  +#include 
   
   #ifdef GAWK
   #undef alloca

So, just move the unconditional inclusion to the start of the compilation
unit root file, before the #include of the regex_internal.h header.

In some ways this is a better fix, because it makes it clear that, currently,
the compat/regex code requires . This would remove the need for
such a comment.

This effectively makes the conditional inclusion of , and the SIZE_MAX
fallback, in regex_internal.h dead code. (The C99 standard _requires_ the
definition of SIZE_MAX in , thankfully! ;-). So, I was tempted to
remove them as part of the patch. However, I also wanted to minimize the changes
to the regex code, just in case we ever wanted to re-import a newer version
from upstream. Setting HAVE_STDINT_H seemed like a good solution, but maybe the
first patch would be more honest?

As I said earlier, I was a little concerned about the 'unconditional' aspect of
the inclusion of . At one time we wanted to support systems that 
didn't
have  (or didn't have  but did have ). However,
it has been several months and we have not heard anyone scream, so ...

It is slightly amusing that the reason you #included  was to get the
definition of 'intptr_t' and the C standard states that this type is optional.
In practice, I suspect that the number of platforms which do not define 
'intptr_t'
and 'uintptr_t' in the  header is rather small.

Having said that ... If I'm reading the code/config correctly, HP-NONSTOP would
be failing to compile at the moment. (Although it has , it does not
define 'intptr_t' - ie it defines 'NO_REGEX = YesPlease' and 'NO_INTPTR_T = \
UnfortunatelyYes').

> Other than that, I think this patch is an improvement.

Thanks. What do you think of replacing it with the original patch (above)?

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regex: fix a SIZE_MAX macro redefinition warning

[Johannes Schindelin]

Hi Ramsay,

thanks for working on this!

On Sat, 4 Jun 2016, Ramsay Jones wrote:

> diff --git a/Makefile b/Makefile
> index 0d59718..3f6c70a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1987,7 +1987,7 @@ endif
>  
>  ifdef NO_REGEX
>  compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
> - -DGAWK -DNO_MBSUPPORT
> + -DGAWK -DNO_MBSUPPORT -DHAVE_STDINT_H

Maybe a comment here, something like "the fallback regex implementation
*requires* stdint.h"?

Other than that, I think this patch is an improvement.

Thanks again,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] regex: fix a SIZE_MAX macro redefinition warning

[Ramsay Jones]

Since commit 56a1a3ab ("Silence GCC's \"cast of pointer to integer of a
different size\" warning", 26-10-2015), sparse has been issuing a macro
redefinition warning for the SIZE_MAX macro. However, gcc did not issue
any such warning.

After commit 56a1a3ab, in terms of the order of #includes and #defines,
the code looked something like:

  $ cat -n junk.c
   1#include 
   2
   3#define SIZE_MAX ((size_t) -1)
   4
   5#include 
   6
   7int main(int argc, char *argv[])
   8{
   9return 0;
  10}
  $
  $ gcc junk.c
  $

However, if you compile that file with -Wsystem-headers, then it will
also issue a warning. Having set -Wsystem-headers in CFLAGS, using the
config.mak file, then (on cygwin):

  $ make compat/regex/regex.o
  CC compat/regex/regex.o
  In file included from 
/usr/lib/gcc/x86_64-pc-cygwin/4.9.3/include/stdint.h:9:0,
   from compat/regex/regcomp.c:21,
   from compat/regex/regex.c:77:
  /usr/include/stdint.h:362:0: warning: "SIZE_MAX" redefined
   #define SIZE_MAX (__SIZE_MAX__)
   ^
  In file included from compat/regex/regex.c:69:0:
  compat/regex/regex_internal.h:108:0: note: this is the location of the 
previous definition
   # define SIZE_MAX ((size_t) -1)
   ^
  $

The compilation of the compat/regex code is somewhat unusual in that the
regex.c file directly #includes the other c files (regcomp.c, regexec.c
and regex_internal.c). Commit 56a1a3ab added an #include of 
to the regcomp.c file, which results in the redefinition, since this is
included after the regex_internal.h header. This header file contains a
'fallback' definition for SIZE_MAX, in order to support systems which do
not have the  header (the HAVE_STDINT_H macro is not defined).

In order to suppress the warning, we remove the #include of 
from regcomp.c and set the HAVE_STDINT_H macro, using the regex.o build
rule within the Makefile, to ensure that  is #included from
the regex_internal.h header.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

About seven months ago, sparse started complaining but, because gcc wasn't, I
just assumed this was a sparse bug. I put it on my sparse TODO list and pretty
much forgot about it. Tonight I decided to take a quick look and was a bit
surprised by what I found. ;-)

I spent some time worrying about the 'unconditional inclusion' of 
(there used to be systems that didn't supply that header that we wanted to
support), but I suspect that is now a non-issue. In any event, I don't
think this patch makes things any worse.

[Just a flavour of the rabbit holes I went down; HP-NONSTOP seems to supply
both  and  but it doesn't supply the intptr_t and
uintptr_t types (ie it defines NO_INTPTR_T). So, I checked the C99 and C11
standards and, blow me down, but the standard states that those types are
_optional_. (C99 7.18.1.4) :-D ]

ATB,
Ramsay Jones

 Makefile   | 2 +-
 compat/regex/regcomp.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 0d59718..3f6c70a 100644
--- a/Makefile
+++ b/Makefile
@@ -1987,7 +1987,7 @@ endif
 
 ifdef NO_REGEX
 compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
-   -DGAWK -DNO_MBSUPPORT
+   -DGAWK -DNO_MBSUPPORT -DHAVE_STDINT_H
 endif
 
 ifdef USE_NED_ALLOCATOR
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index fba5986..d8bde06 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -18,8 +18,6 @@
Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA.  */
 
-#include 
-
 static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
  size_t length, reg_syntax_t syntax);
 static void re_compile_fastmap_iter (regex_t *bufp,
-- 
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html