I based my question on the fact that the majority of the cases seem to
explicitely check against the macro.
Oh well, should've waited before patching. It was a nice way of
getting the overall feel of the library though - nice work!
On Mon, 27 Dec 2004 09:44:06 -0500, Garrett Rooney
<[EMAIL PROTECTED]> wrote:
> Mihai Limbasan wrote:
> >>From what I've gathered reviewing the APR code, the canonical way of
> > testing apr_* return values is comparing against APR_SUCCESS. That
> > happens to be #defined as zero.
> >
> > There are a lot of places around the APR codebase that do stuff like this:
> >
> > if (rv)
> > return rv;
> >
> > i.e. taking advantage of the knowledge that APR_SUCCESS is zero and
> > returning (or taking some other action) based on a non-zero (hence
> > non-APR_SUCCESS) status.
> >
> > This seems to defeat the point of having an APR_SUCCESS in the first
> > place. It's also (at least in my eyes) slightly less intuitive than
> > explicit testing.
>
> Part of the definition of apr_status_t is the fact that APR_SUCCESS is
> defined to be zero. This is by design, because success is the common
> case you test for, and comparing against zero is fast for most
> processors. Because it will always be this way there's no reason not to
> publicise the fact, making the "if (rv) blah" idiom the common way to
> deal with errors in code that uses APR.
>
> > Would it be useful if I went through the code and properly
> > canonicalized such constructs to
> >
> > if (rv != APR_SUCCESS) {
> > return rv;
> > }
> >
>
> Once you read the documentation and learn the idiom the "if (rv) return
> rv;" way is much easier to read IMO, I think your change would actually
> make things less clear in the long run.
>
> -garrett
>
--
Mihai Limbasan
