Re: [PATCH] Fixes sentinel warnings on GCC4

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Neil Williams-2
On Sunday 04 December 2005 1:45 pm, Sjoerd Langkemper wrote:
> This patch fixes warnings like these:
> warning: missing sentinel in function call

What system are you using to build? Gcc4 has been in use by various developers
for some time without any of these warnings - they would have been noticed as
they would have halted the build. Is this 64-bit only?

> Look at this page to find at what this warning is about:
> http://www.linuxonly.nl/docs/sentinel/

I have, but the patch still doesn't make sense to me.

> This patch basically replaces NULL with (char *)NULL

I may be wrong, but that looks like a cast for no good reason and that has
caused problems before.

>          src/backend/file/gnc-backend-file.c
-    be->lockfile = g_strconcat(be->fullpath, ".LCK", NULL);
+    be->lockfile = g_strconcat(be->fullpath, ".LCK", (char *)NULL);

Sorry, to me, that looks plain wrong. The glib API for g_strconcat clearly
states NULL not (char*)NULL:
http://developer.gnome.org/doc/API/2.0/glib/glib-String-Utility-Functions.html#g-strconcat

"The variable argument list must end with NULL. If you forget the NULL,
g_strconcat() will start appending random memory junk to your string.
string1 :  The first string to add, which must not be NULL.
  ... :  a NULL-terminated list of strings to append to the string"

Absolutely no mention of (char*)NULL.

If glib required (char*)NULL, the API would mention it and provide a suitable
#define for the cast.

I fear this patch could break a lot more than it purports to fix.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Sun, Dec 04, 2005 at 02:45:42PM +0100, Sjoerd Langkemper wrote:

> This patch fixes warnings like these:
> warning: missing sentinel in function call
>
> Look at this page to find at what this warning is about:
> http://www.linuxonly.nl/docs/sentinel/
>
> This patch basically replaces NULL with (char *)NULL in functions which
> are NULL-terminated. Maybe it is a good idea to define a constant for
> (char *)NULL, but I don't know where that would be defined.
>
> Sjoerd

Patch looks right except for some of :

+  new_map = g_strconcat(map, accel_key, (gchar *)(char *)NULL);

(I think I saw about 5.)

and several uses in comparisons instead of variadics:

+(clean_url != (char *)NULL) ? (char const *)clean_url : url,

which are not needed or desirable.

BTW, there's a small error in the explanation at the website.  "NULL"
as argument to variadic function is not typed (void *).  (If it were,
there wouldn't be a problem here, err at least not *this* problem.  :)

A resubmission would be greatly appreciated.  Thanks.

-chris

Meta-comment for list maintainer:

  Can we please set ReplyTo on -patches mail to be gnucash-devel?

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Derek Atkins
In reply to this post by Neil Williams-2
If you were on IRC you'd have more information on this.

Yes, it is 64-bit only.  The problem is that on some versions of gcc4
stddef.h #defined NULL as (int)0 which causes a compiler warning when
passing that into a char*.

The fix (which David is working on -- which you'd know if you were in
IRC) is to #define GNC_NULL ((gpointer)0) and then use that instead of
NULL in these places.

-derek

Quoting Neil Williams <[hidden email]>:

> On Sunday 04 December 2005 1:45 pm, Sjoerd Langkemper wrote:
>> This patch fixes warnings like these:
>> warning: missing sentinel in function call
>
> What system are you using to build? Gcc4 has been in use by various
> developers
> for some time without any of these warnings - they would have been noticed as
> they would have halted the build. Is this 64-bit only?
>
>> Look at this page to find at what this warning is about:
>> http://www.linuxonly.nl/docs/sentinel/
>
> I have, but the patch still doesn't make sense to me.
>
>> This patch basically replaces NULL with (char *)NULL
>
> I may be wrong, but that looks like a cast for no good reason and that has
> caused problems before.
>
>>          src/backend/file/gnc-backend-file.c
> -    be->lockfile = g_strconcat(be->fullpath, ".LCK", NULL);
> +    be->lockfile = g_strconcat(be->fullpath, ".LCK", (char *)NULL);
>
> Sorry, to me, that looks plain wrong. The glib API for g_strconcat clearly
> states NULL not (char*)NULL:
> http://developer.gnome.org/doc/API/2.0/glib/glib-String-Utility-Functions.html#g-strconcat
>
> "The variable argument list must end with NULL. If you forget the NULL,
> g_strconcat() will start appending random memory junk to your string.
> string1 :  The first string to add, which must not be NULL.
>  ... :  a NULL-terminated list of strings to append to the string"
>
> Absolutely no mention of (char*)NULL.
>
> If glib required (char*)NULL, the API would mention it and provide a suitable
> #define for the cast.
>
> I fear this patch could break a lot more than it purports to fix.
>
> --
>
> Neil Williams
> =============
> http://www.data-freedom.org/
> http://www.nosoftwarepatents.com/
> http://www.linux.codehelp.co.uk/
>
>



--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Derek Atkins
In reply to this post by Chris Shoemaker
Quoting Chris Shoemaker <[hidden email]>:

> Meta-comment for list maintainer:
>
>  Can we please set ReplyTo on -patches mail to be gnucash-devel?

Done.

-derek

--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
In reply to this post by Derek Atkins
On Mon, Dec 05, 2005 at 04:38:40PM -0500, Derek Atkins wrote:
> If you were on IRC you'd have more information on this.
>
> Yes, it is 64-bit only.  The problem is that on some versions of gcc4
> stddef.h #defined NULL as (int)0 which causes a compiler warning when
> passing that into a char*.

I'm no expert, and I don't actually know what gcc #defines NULL to,
but I don't think this is quite right.  I don't think this is a gcc
problem.  No compiler can infer the correct type for variadic
arguments.  In a variadic argument list, a compiler *must* interpret
an unqualified "0" (which is probably what NULL is) as integer type.
Our code is just wrong for any machine where the null pointer is not a
32 bit zero, even if it's some (imaginary) 32-bit machine.

>
> The fix (which David is working on -- which you'd know if you were in
> IRC) is to #define GNC_NULL ((gpointer)0) and then use that instead of
> NULL in these places.

Hmm.  Seems a bit heavy-handed to me, but it would work.  I hope it
doesn't result in gratuitous use of GNC_NULL in the 99% of places
where NULL is fine.  

IMHO, it's better that programmers remain aware that the use of NULL
as a sentinel is "special".  IOW, *not* hiding this behavior gives a
better picture of what the code actually does, which tends to make for
better code and better programmers in the long run.  One problem I see
with GNC_NULL, is visibility.  If it's too visible, people use it
gratuitously; if it's not visible enough, people forget to use it when
they should; How to choose?  Better just to /understand/ what's going
on.

-chris

_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> On Mon, Dec 05, 2005 at 04:38:40PM -0500, Derek Atkins wrote:
>> If you were on IRC you'd have more information on this.
>>
>> Yes, it is 64-bit only.  The problem is that on some versions of gcc4
>> stddef.h #defined NULL as (int)0 which causes a compiler warning when
>> passing that into a char*.
>
> I'm no expert, and I don't actually know what gcc #defines NULL to,
> but I don't think this is quite right.  I don't think this is a gcc
> problem.  No compiler can infer the correct type for variadic
> arguments.  In a variadic argument list, a compiler *must* interpret
> an unqualified "0" (which is probably what NULL is) as integer type.
> Our code is just wrong for any machine where the null pointer is not a
> 32 bit zero, even if it's some (imaginary) 32-bit machine.

The problem is that you cannot pass a 32-bit integer into a 64-bit adddress.
On my system (32-bit, gcc-3.4.3) NULL is either defined as 0, (void*)0, or
__null depending on other #ifdefs (which I didn't go examine).  However
if the callee is expecting a 64-bit pointer and you pass it a 32-bit
integer, the compiler WILL (and SHOULD) complain.  With -Werror this
causes the build to blow out.

>> The fix (which David is working on -- which you'd know if you were in
>> IRC) is to #define GNC_NULL ((gpointer)0) and then use that instead of
>> NULL in these places.
>
> Hmm.  Seems a bit heavy-handed to me, but it would work.  I hope it
> doesn't result in gratuitous use of GNC_NULL in the 99% of places
> where NULL is fine.

I think it would only be used in the particular cases where this is an
issue.  HOWEVER....

> IMHO, it's better that programmers remain aware that the use of NULL
> as a sentinel is "special".  IOW, *not* hiding this behavior gives a
> better picture of what the code actually does, which tends to make for
> better code and better programmers in the long run.  One problem I see
> with GNC_NULL, is visibility.  If it's too visible, people use it
> gratuitously; if it's not visible enough, people forget to use it when
> they should; How to choose?  Better just to /understand/ what's going
> on.

... we're still trying to analyze the issue and figure out what's really
going on.  I.e., there's no plan to make any changes until we understand
the problem.  David just went and tried to reproduce this on an amd64
machine and could not reproduce it.  Then we hear back that these errors
are on a 32-bit machine..  (all this is on IRC, and all in the last
few minutes)..   So..  We're still trying to figure out what the REAL
problem is before we consider this a gnucash  bug.

I agree with you that this SHOULDN'T be a problem, and we SHOULDN'T
need to make this change.

> -chris

-derek

--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-patches mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-patches
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Mon, Dec 05, 2005 at 05:26:34PM -0500, Derek Atkins wrote:

> Quoting Chris Shoemaker <[hidden email]>:
>
> >On Mon, Dec 05, 2005 at 04:38:40PM -0500, Derek Atkins wrote:
> >>If you were on IRC you'd have more information on this.
> >>
> >>Yes, it is 64-bit only.  The problem is that on some versions of gcc4
> >>stddef.h #defined NULL as (int)0 which causes a compiler warning when
> >>passing that into a char*.
> >
> >I'm no expert, and I don't actually know what gcc #defines NULL to,
> >but I don't think this is quite right.  I don't think this is a gcc
> >problem.  No compiler can infer the correct type for variadic
> >arguments.  In a variadic argument list, a compiler *must* interpret
> >an unqualified "0" (which is probably what NULL is) as integer type.
> >Our code is just wrong for any machine where the null pointer is not a
> >32 bit zero, even if it's some (imaginary) 32-bit machine.
>
> The problem is that you cannot pass a 32-bit integer into a 64-bit adddress.
> On my system (32-bit, gcc-3.4.3) NULL is either defined as 0, (void*)0, or
> __null depending on other #ifdefs (which I didn't go examine).  However
> if the callee is expecting a 64-bit pointer and you pass it a 32-bit
> integer, the compiler WILL (and SHOULD) complain.  With -Werror this
> causes the build to blow out.

This is good.

>
> >>The fix (which David is working on -- which you'd know if you were in
> >>IRC) is to #define GNC_NULL ((gpointer)0) and then use that instead of
> >>NULL in these places.
> >
> >Hmm.  Seems a bit heavy-handed to me, but it would work.  I hope it
> >doesn't result in gratuitous use of GNC_NULL in the 99% of places
> >where NULL is fine.
>
> I think it would only be used in the particular cases where this is an
> issue.  HOWEVER....
>
> >IMHO, it's better that programmers remain aware that the use of NULL
> >as a sentinel is "special".  IOW, *not* hiding this behavior gives a
> >better picture of what the code actually does, which tends to make for
> >better code and better programmers in the long run.  One problem I see
> >with GNC_NULL, is visibility.  If it's too visible, people use it
> >gratuitously; if it's not visible enough, people forget to use it when
> >they should; How to choose?  Better just to /understand/ what's going
> >on.
>
> ... we're still trying to analyze the issue and figure out what's really
> going on.  I.e., there's no plan to make any changes until we understand
> the problem.  David just went and tried to reproduce this on an amd64
> machine and could not reproduce it.  Then we hear back that these errors
> are on a 32-bit machine..  (all this is on IRC, and all in the last
> few minutes)..   So..  We're still trying to figure out what the REAL
> problem is before we consider this a gnucash  bug.
>
> I agree with you that this SHOULDN'T be a problem, and we SHOULDN'T
> need to make this change.

You misunderstand me.  This *is* a problem, and we *should* fix it.
Our code is just *wrong*.  It's wrong on 32-bit machines and on 64-bit
machines.  (Although it happens to be a benign bug anywhere ((char *)
NULL) is a 32-bit zero, which is almost everywhere.)  It's just wrong
C.  '0' is only equivalent to the typed null pointer in a pointer
context (that's even true when a typed null pointer is not a 32-bit
null; it's part of the language definition of a null pointer.)  An
argument list to a variadic function is no different than a function
with no prototype - the arguments are NOT in pointer context.

The C-language rule is "always cast NULL when used as a sentinel to a
variadic function, otherwise you're accidentally passing integer
zero, which is wrong."

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Mon, Dec 05, 2005 at 06:28:00PM -0500, Chris Shoemaker wrote:
> You misunderstand me.  This *is* a problem, and we *should* fix it.
> Our code is just *wrong*.  It's wrong on 32-bit machines and on 64-bit
> machines.  (Although it happens to be a benign bug anywhere ((char *)
> NULL) is a 32-bit zero, which is almost everywhere.)  It's just wrong
> C.  '0' is only equivalent to the typed null pointer in a pointer
> context (that's even true when a typed null pointer is not a 32-bit
> null; it's part of the language definition of a null pointer.)  An
 ^^^^^
  err, that should be "zero".  I can't seem to find a online copy of
the ANSI C standard but google turned up several portability guides
that make this same point.  Here's one:
http://www.psgd.org/paul/docs/cport/cport11.htm

Notice that constant zero is not always equal to a value of zero.

-chris

> argument list to a variadic function is no different than a function
> with no prototype - the arguments are NOT in pointer context.
>
> The C-language rule is "always cast NULL when used as a sentinel to a
> variadic function, otherwise you're accidentally passing integer
> zero, which is wrong."
>
> -chris
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

David Hampton-2
In reply to this post by Chris Shoemaker
On Mon, 2005-12-05 at 18:28 -0500, Chris Shoemaker wrote:
> On Mon, Dec 05, 2005 at 05:26:34PM -0500, Derek Atkins wrote:
> >
> > ... we're still trying to analyze the issue and figure out what's really
> > going on.  I.e., there's no plan to make any changes until we understand
> > the problem.  David just went and tried to reproduce this on an amd64
> > machine and could not reproduce it.  Then we hear back that these errors
> > are on a 32-bit machine..  (all this is on IRC, and all in the last
> > few minutes)..   So..  We're still trying to figure out what the REAL
> > problem is before we consider this a gnucash  bug.

I checked the 32 bit Intel and 64 bit AMD systems that I have access to.
On all of these machines NULL is defined as "((void *)0)", and they all
compile gnucash cleanly.  The only way I can force these errors to occur
is to throw away the existing definition of NULL (as defined
in /usr/lib/gcc/<xxx>/4.0.2/include/stddef.h) and explicitly redefine
NULL to be an untyped "0".  Even then, I only see the problem occur when
compiling on an AMD64 system.  For that matter, to even elicit the error
I have to redefine NULL after all the system header files have been
included, or it is forced it back to the "((void*)0)" definition.

I have no explanation why Sjoerd is seeing this problem on a 32bit
system.  The only theory I can come up with is that his compiler is
treating C files as C++ files, and is thereby using a definition of NULL
as an untyped zero.  He would then also have to be compiling with the
-Wstrict-null-sentinel argument, which is only allowed for C++ compiles
and is specifically not included in the -Wall argument.

> You misunderstand me.  This *is* a problem, and we *should* fix it.

No, its not a problem and we do not need to fix anything.

> The C-language rule is "always cast NULL when used as a sentinel to a
> variadic function, otherwise you're accidentally passing integer
> zero, which is wrong."

And as far as I can tell that's exactly what the GCC compiler provides
when compiling C code.  On 32 bit systems and 64 bit systems.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Mon, Dec 05, 2005 at 08:39:15PM -0500, David Hampton wrote:

> On Mon, 2005-12-05 at 18:28 -0500, Chris Shoemaker wrote:
> > On Mon, Dec 05, 2005 at 05:26:34PM -0500, Derek Atkins wrote:
> > >
> > > ... we're still trying to analyze the issue and figure out what's really
> > > going on.  I.e., there's no plan to make any changes until we understand
> > > the problem.  David just went and tried to reproduce this on an amd64
> > > machine and could not reproduce it.  Then we hear back that these errors
> > > are on a 32-bit machine..  (all this is on IRC, and all in the last
> > > few minutes)..   So..  We're still trying to figure out what the REAL
> > > problem is before we consider this a gnucash  bug.
>
> I checked the 32 bit Intel and 64 bit AMD systems that I have access to.
> On all of these machines NULL is defined as "((void *)0)", and they all
> compile gnucash cleanly.  The only way I can force these errors to occur
> is to throw away the existing definition of NULL (as defined
> in /usr/lib/gcc/<xxx>/4.0.2/include/stddef.h) and explicitly redefine
> NULL to be an untyped "0".  Even then, I only see the problem occur when
> compiling on an AMD64 system.  For that matter, to even elicit the error
> I have to redefine NULL after all the system header files have been
> included, or it is forced it back to the "((void*)0)" definition.

If NULL is ((void*)0) you won't have any problem using NULL as a
sentinel, but using that definition of NULL opens up the possibility
of writing code that works with one implementation and not with
another.  Specifically, you couldn't say "int *i = ((int *) NULL)" and
be sure it would mean the same thing for every compiler.

> I have no explanation why Sjoerd is seeing this problem on a 32bit
> system.  The only theory I can come up with is that his compiler is
> treating C files as C++ files, and is thereby using a definition of NULL
> as an untyped zero.  He would then also have to be compiling with the
> -Wstrict-null-sentinel argument, which is only allowed for C++ compiles
> and is specifically not included in the -Wall argument.
>
> > You misunderstand me.  This *is* a problem, and we *should* fix it.
>
> No, its not a problem and we do not need to fix anything.
>
> > The C-language rule is "always cast NULL when used as a sentinel to a
> > variadic function, otherwise you're accidentally passing integer
> > zero, which is wrong."
>
> And as far as I can tell that's exactly what the GCC compiler provides
> when compiling C code.  On 32 bit systems and 64 bit systems.

Do we want to ship code that only works with a system that decides to
helpfully #define NULL ((void*)0), when another system may very
correctly #define NULL 0?  

The #define NULL 0 case is explicitly allowed by the ISO C standard,
and it's easy to write portable code that works for that case, so why
wouldn't we?

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

David Hampton-2
On Mon, 2005-12-05 at 21:25 -0500, Chris Shoemaker wrote:

> The #define NULL 0 case is explicitly allowed by the ISO C standard,
> and it's easy to write portable code that works for that case, so why
> wouldn't we?

I don't know for sure, but I strongly suspect that our code is already
limited to compiling with the gnu tool chain.  Given that gcc seems to
do the right thing where NULL is concerned, I feel no need to do
anything more than add a note that gcc is required to compile gnucash.
I've got too many other dragons to slay.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Mon, Dec 05, 2005 at 10:09:10PM -0500, David Hampton wrote:

> On Mon, 2005-12-05 at 21:25 -0500, Chris Shoemaker wrote:
>
> > The #define NULL 0 case is explicitly allowed by the ISO C standard,
> > and it's easy to write portable code that works for that case, so why
> > wouldn't we?
>
> I don't know for sure, but I strongly suspect that our code is already
> limited to compiling with the gnu tool chain.  Given that gcc seems to
> do the right thing where NULL is concerned, I feel no need to do
> anything more than add a note that gcc is required to compile gnucash.
> I've got too many other dragons to slay.

Fair enough.  I'll grab the original patch, remove redundant portions
and apply it.  I'm convinced it's correct.  I figure it's easier to
follow the standard than to figure out what weird systems people
compile GnuCash on.  ( I don't even want to hazard a guess. )

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> On Mon, Dec 05, 2005 at 10:09:10PM -0500, David Hampton wrote:
>> On Mon, 2005-12-05 at 21:25 -0500, Chris Shoemaker wrote:
>>
>> > The #define NULL 0 case is explicitly allowed by the ISO C standard,
>> > and it's easy to write portable code that works for that case, so why
>> > wouldn't we?
>>
>> I don't know for sure, but I strongly suspect that our code is already
>> limited to compiling with the gnu tool chain.  Given that gcc seems to
>> do the right thing where NULL is concerned, I feel no need to do
>> anything more than add a note that gcc is required to compile gnucash.
>> I've got too many other dragons to slay.
>
> Fair enough.  I'll grab the original patch, remove redundant portions
> and apply it.  I'm convinced it's correct.  I figure it's easier to
> follow the standard than to figure out what weird systems people
> compile GnuCash on.  ( I don't even want to hazard a guess. )

Is the patch actually required?   Nobody else can reproduce the
problem on 32- or 64-bit platforms...

> -chris

-derek
--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

David Hampton-2
In reply to this post by Chris Shoemaker
On Mon, 2005-12-05 at 22:19 -0500, Chris Shoemaker wrote:

> Fair enough.  I'll grab the original patch, remove redundant portions
> and apply it.  I'm convinced it's correct.  I figure it's easier to
> follow the standard than to figure out what weird systems people
> compile GnuCash on.  ( I don't even want to hazard a guess. )

I'm not concerned about whether the patch is correct (based on his last
patch it probably is) but whether its necessary.  Also, there's no
automated way to maintain the insistence that sentinels are explicitly
written as "(void *)NULL" [which is probably a safer cast than "(char
*)NULL"].   Sjoerd seems to have the only system that reports an uncast
NULL as an error.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Chris Shoemaker
On Mon, Dec 05, 2005 at 10:37:49PM -0500, David Hampton wrote:
> On Mon, 2005-12-05 at 22:19 -0500, Chris Shoemaker wrote:
>
> > Fair enough.  I'll grab the original patch, remove redundant portions
> > and apply it.  I'm convinced it's correct.  I figure it's easier to
> > follow the standard than to figure out what weird systems people
> > compile GnuCash on.  ( I don't even want to hazard a guess. )
>
> I'm not concerned about whether the patch is correct (based on his last
> patch it probably is) but whether its necessary.  

In the gcc ML thread where the -Wstrict-null-sentinel warning was
added, it was claimed that at least some versions of HPUX and Solaris
2.8 on 64-bit machines #define NULL 0.

> Also, there's no
> automated way to maintain the insistence that sentinels are explicitly
> written as "(void *)NULL" [which is probably a safer cast than "(char
> *)NULL"].  

Well, that's exactly why this warning was recently added to gcc, and
why the glib and gtk+ sources were tagged with attribute((sentinel)).
(Although that also has the benefit of catching no sentinel at all
which is more important than sentinal-of-wrong-size.) As to why it's
currently enabled only for g++, I have no idea.  I've asked on the gcc
ML.

> Sjoerd seems to have the only system that reports an uncast
> NULL as an error.

Maybe he compiled with g++?  Is that even possible?  I wonder...

-chris

>
> David
>
>
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

David Hampton-2
On Tue, 2005-12-06 at 00:55 -0500, Chris Shoemaker wrote:
> Well, that's exactly why this warning was recently added to gcc, and
> why the glib and gtk+ sources were tagged with attribute((sentinel)).
> (Although that also has the benefit of catching no sentinel at all
> which is more important than sentinal-of-wrong-size.) As to why it's
> currently enabled only for g++, I have no idea.  I've asked on the gcc
> ML.

Sentinel checking works across the board in gcc4.  Its strict sentinel
checking that can only be enabled when compiling C++ code.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Karl Hegbloom
In reply to this post by Chris Shoemaker
On Mon, 2005-12-05 at 18:28 -0500, Chris Shoemaker wrote:
> The C-language rule is "always cast NULL when used as a sentinel to a
> variadic function, otherwise you're accidentally passing integer
> zero, which is wrong."

I think that's bogus.  NULL should always be a pointer (void *)0, and if
it's not, then the header that defined it is wrong.  You should not have
to cast it.

At least on my machine, it's defined in <stddef.h> as (void *)0.

  /usr/lib/gcc/i486-linux-gnu/4.0.2/include/stddef.h

I made a cscope index of /usr/include, and am finding definitions of
NULL all over the place.  Hmmm.

--
Karl Hegbloom <[hidden email]>

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Karl Hegbloom
In reply to this post by Chris Shoemaker
On Mon, 2005-12-05 at 21:25 -0500, Chris Shoemaker wrote:
> If NULL is ((void*)0) you won't have any problem using NULL as a
> sentinel, but using that definition of NULL opens up the possibility
> of writing code that works with one implementation and not with
> another.  Specifically, you couldn't say "int *i = ((int *) NULL)" and
> be sure it would mean the same thing for every compiler.

Yes you could.  The whole point of the (void *) type is to represent a
generic pointer that can be assigned to any other pointer type _without_
a cast.  That's why it was invented.  In C, the variable holds the type,
not the value, so the (void *)0 automatically becomes an (int *) without
a cast.

A pointer to a char is the same size as a pointer to an int and is the
same size as a pointer to void.  The only difference is that when you
increment a char*, it clicks by one byte, and when you increment an
int*, it clicks by sizeof(int) bytes.  Since int *i, above, is a pointer
to int, it works as expected even though you initialize it with NULL.

--
Karl Hegbloom <[hidden email]>

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Karl Hegbloom
In reply to this post by Chris Shoemaker
On Tue, 2005-12-06 at 00:55 -0500, Chris Shoemaker wrote:
> In the gcc ML thread where the -Wstrict-null-sentinel warning was
> added, it was claimed that at least some versions of HPUX and Solaris
> 2.8 on 64-bit machines #define NULL 0.

Then those system headers are wrong, not the code that uses NULL as a
(void *)0.

I wonder if there's a way to use 'typeof()' to get a readout as to
whether it needs to be redefined properly?

I don't get how C++ can get away with calling NULL == 0 rather than ==
(void *)0.

--
Karl Hegbloom <[hidden email]>

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixes sentinel warnings on GCC4

Derek Atkins
In reply to this post by Derek Atkins
Derek Atkins <[hidden email]> writes:

> Quoting Chris Shoemaker <[hidden email]>:
>
>> Meta-comment for list maintainer:
>>
>>  Can we please set ReplyTo on -patches mail to be gnucash-devel?
>
> Done.

Er.. I missed a radio-button.   It should /really/ be fixed now.
Sorry.

-derek
--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
12