Re: fixup bogus SetLastError()

[Alexandre Julliard]

Andreas Mohr  writes:
> objects/gdiobj.c really has no business dealing with SetLastError().
> SetLastError() is a mechanism to communicate errors to external programs,
> so it should definitely not be used for highly internal functions.

You didn't include the gdiobj.c patch, but I disagree with it
anyway... There can be many good reasons for setting last error in
internal functions; for instance setting it inside GDI_GetObjPtr
avoids having to set it in every single caller. And in the very rare
case where this is not what we want, the caller can always set it to
another value.

fixup bogus SetLastError()

On Mon, Jun 04, 2001 at 11:11:23AM -0700, Alexandre Julliard wrote:
> Andreas Mohr  writes:
> > objects/gdiobj.c really has no business dealing with SetLastError().
> > SetLastError() is a mechanism to communicate errors to external programs,
> > so it should definitely not be used for highly internal functions.
>
> You didn't include the gdiobj.c patch, but I disagree with it

Hmm, strange. I did a diff with the files fetched from a pre-arranged
file list, and I'm damn sure that I had included that file there :-\
> anyway... There can be many good reasons for setting last error in
> internal functions; for instance setting it inside GDI_GetObjPtr
> avoids having to set it in every single caller. And in the very rare
> case where this is not what we want, the caller can always set it to
> another value.

Now that's where I disagree !
The caller can *not* always set it to another value.
If it does and the program intends to read the LastError of a *previously*
executed API that failed, then the LastError will be reset even though
it shouldn't have been !
(I know, programs that don't read the LastError immediately after function
return are braindead, but then we're writing for the Windows platform,
so we have lots of braindead programs anyway, right ? ;-)

In short: the SetLastError behaviour needs to match Windows behaviour very,
very closely.
And IMHO GDI_GetObjPtr() is such a widely used function that this will set
LastError 6 in every case of a NULL handle, so this will spoil a lot of
innocent error settings...
Of course one could argue that it should be the calling function that should
be fixed, then (to not pass a NULL handle to GDI_GetObjPtr).
This would be SelectObject() in this case, BTW.

What to do ?

fixup bogus SetLastError()

[Marcus Meissner]

On Mon, Jun 04, 2001 at 08:31:06PM +0200, Andreas Mohr wrote:
> On Mon, Jun 04, 2001 at 11:11:23AM -0700, Alexandre Julliard wrote:
> > Andreas Mohr  writes:
> > > objects/gdiobj.c really has no business dealing with SetLastError().
> > > SetLastError() is a mechanism to communicate errors to external programs,
> > > so it should definitely not be used for highly internal functions.
> >
> > You didn't include the gdiobj.c patch, but I disagree with it
>
> Hmm, strange. I did a diff with the files fetched from a pre-arranged
> file list, and I'm damn sure that I had included that file there :-\
> > anyway... There can be many good reasons for setting last error in
> > internal functions; for instance setting it inside GDI_GetObjPtr
> > avoids having to set it in every single caller. And in the very rare
> > case where this is not what we want, the caller can always set it to
> > another value.
>
> Now that's where I disagree !
> The caller can *not* always set it to another value.
> If it does and the program intends to read the LastError of a *previously*
> executed API that failed, then the LastError will be reset even though
> it shouldn't have been !

There are only _some_ cases that do this.
> (I know, programs that don't read the LastError immediately after function
> return are braindead, but then we're writing for the Windows platform,
> so we have lots of braindead programs anyway, right ? ;-)
> In short: the SetLastError behaviour needs to match Windows behaviour very,
> very closely.
> And IMHO GDI_GetObjPtr() is such a widely used function that this will set
> LastError 6 in every case of a NULL handle, so this will spoil a lot of
> innocent error settings...
> Of course one could argue that it should be the calling function that should
> be fixed, then (to not pass a NULL handle to GDI_GetObjPtr).
> This would be SelectObject() in this case, BTW.
>
> What to do ?

We need to evaluate LastError behaviour by function anyway.

So if a function misbehaves, or sets the last error erroneously, fix it.
Otherwise just do not care.

Ciao, Marcus