Re: cvs commit: src/usr.bin/newgrp

[Max Okumoto <okumoto@xxxxxxxx>]

Devon H. O'Dell wrote:
> Matthew Dillon wrote:
> > :liamfoy     2004/12/08 14:00:32 PST
> > :
> > :DragonFly src repository
> > :
> > :  Modified files:
> > :    usr.bin/newgrp       newgrp.c :  Log:
> > :  - Check the return value of setenv(). We should check this value since
> > :    setenv() uses both malloc and realloc.
> > :  :  Revision  Changes    Path
> > :  1.2       +9 -3      src/usr.bin/newgrp/newgrp.c
> >
> >
> >     That's fine, though the typical way of checking the return     
> > value is to check for it < 0 rather then exactly equal to
> >     -1.  Both are correct, but < 0 is used the most often in the code
> >     base and considered easier to read.
> >
> >                     -Matt
> >                     Matthew Dillon                     
> > <dillon@xxxxxxxxxxxxx>
>
> Is it? I always found == -1 to be rather straightforward, and 
> considering (according to SUSv3) that setenv _must_ return only 0 or -1, 
> I see no point to check for any other value. If setenv were to return 
> anything else on failure, and you would need to check what the error 
> was, I could understand this. But errno is set when setenv errors, and 
> returns -1, so I'm stumped as to why checking for < 0 is considered to 
> be easier to read, since it seems more like an obfuscation to what the 
> case actually can be in reality.
>
> Kind regards,
>
> Devon H. O'Dell

Alot of the code that I have seen uses the < 0 idiom.  Negative numbers
are commonly used to indicate error, but in most cases it just dosen't
matter which error has occured.  Testing of == indicates to me that
you are testing for a spacific error.  But doing that slows down my
reading of the code since now I have to see which other errors might
be returned.  Yes you know that setenv() and alot of other functions
return only -1, but some return other negative numbers.  So now I have
to go check if setenv() returns other errors.

Using < 0 tells me that if any error value is returned, handle it here.

					Max