From what I've gathered reviewing the APR code, the canonical way oftesting 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
