build failure on netbsd-5 with gtk2+-2.20.1nb

[Bastien Nocera]

On Wed, 2010-06-30 at 08:19 -0400, Greg Troxel wrote:
> Bastien Nocera <hadess at hadess.net> writes:
> 
> > On Sat, 2010-06-26 at 08:19 -0400, Greg Troxel wrote: 
> >> Bastien Nocera <hadess at hadess.net> writes:
> > <snip> 
> >> > Did you compile glib (or did you get a glib from your distro) without
> >> > warnings enabled?
> >> 
> >> glib2 is from pkgsrc, and I am pretty sure it doesn't disable warnings.
> >> 
> >> I read gmessage.h and I see what you mean.  However, it's reasonable for
> >> a compiler to err on the safe side and complain about a variable which
> >> is not 100% clearly only used when initialized.
> >> 
> >> Plus, unless geoclue checks and fails to build if warnings are off, it
> >> needs to check properly. It seems g_return_val_if_fail is meant to
> >> support eiffel-style design-by-contract, and to document the rules
> >> (always) and enforce them (warnings on, which seems default).
> >> 
> >> It seems easy enough to set the offending variable to NULL to avoid
> >> this.
> >
> > Except that's working around a compiler bug. You're more than welcome
> > patching this for your distribution of the package, but I don't see why
> > we should put work-arounds that might hide bugs later in geoclue. 
> 
> A simple assignment to NULL will not hide bugs; it just causes the
> routine to return NULL instead of undefined if for some reason the
> g_return_val_if_fail invocation doesn't do what is expected.

It will hide bugs if we get one of the condition wrong in the loop
below, or forget to update the guard at the top of the function.

> It's not really a compiler bug; it's a failure to do enough static
> analysis to show that something that is on its face unsafe is actually
> ok.

Yeah, which is a bug in the version of gcc your netbsd ships.

> (If you're really concerned about the future and bugs, it would be good
> to have comments documenting the preconditions and postconditions of
> procedures.  And perhaps abort() rather than continuing when they are
> violated.)

They don't continue. The g_return_val_if_fail() will print a warning and
return if the condition is not met.

> Patch attached, or just set it to NULL on declaration without a comment
> (which is what I would do).  I don't see how this can cause problems for
> anyone.

Again, apply this downstream, fix the compiler, or build without
-Werror.

I'm not taking this patch.

Cheers