On Mon, 18 Apr 2005, Stas Bekman wrote:
Randy Kobes wrote:
In the discussion of a failure on Win32 of t/error/runtime at http://marc.theaimsgroup.com/?t=111217856900001&r=1&w=2, Stas suggested adding an APR::Status, containing things like APR_STATUS_IS_EAGAIN(s) from apr_errno.h. I've attached a patch against the current mp2 svn implementing such a thing, which includes all the APR_STATUS_IS_* macros available for the 'error' codes of APR::Const in Apache2::ConstantsTable. The patch includes some tests, as well as a modification to t/response/TestError/runtime.pm using APR::Status; with this, the t/error/runtime test passes on Win32.
Thanks Randy for working on this one :)
Do we really want to expose all those macros? I bet some of those will never be used since we don't expose the APIs which can possibly trigger those error conditions. Exposing APIs comes with maintenance burden. So I'd suggest exposing only those we know are needed. i.e. IS_EAGAIN, and may be some others that we should decide on. Especially notice that most macros are not needed, as they are not composites, e.g.:
#define APR_STATUS_IS_EOF(s) ((s) == APR_EOF)
which can already be easily done with what we have.
If it was me deciding I'd just start by adding IS_EAGAIN and add others as we discover the need. If you can see something else that will certainly be useful in the future, please suggest which.
That's true that some of them aren't composites, but of the 56 APR_E* constants that APR::Const provides, I counted 27 composites (sometimes only on certain platforms). The tests have encountered only IS_EAGAIN, but I thought conceivably the others may also be needed in some circumstances, and since there were so many, I thought it just as easy to do them all at once (also, at some time in the future apr may change some non-composite APR_STATUS_IS_* to a composite).
I most likely don't appreciate the implications of your
comment about exposing the API that may trigger a particular
error - if we supply an APR::Const::EFOOBAR, then is there
any added maintenance burden in also supplying an
APR_STATUS_IS_EFOOBAR(s) macro?
That's not what I've said or meant to say. I say that we certainly *should not* provide error codes/macros for APIs that we don't expose. And in reverse we *should provide* error codes/macros for APIs that we do expose. Though I've suggested that unless someone wants to review what error macros are required, we start with what we know we need.
Re: APR::Const::EFOOBAR vs. APR_STATUS_IS_EFOOBAR(s), it's a tricky situation. If both are exposed how does the user know which one they need to use? e.g. in the case of EAGAIN you can see that a developer working on unix can use just APR::Const::EAGAIN, so it won't work on win32, so APR_STATUS_IS_EAGAIN needs to be used. So may be we need to abolish the constant APR::Const::EAGAIN to make sure users use APR_STATUS_IS_EAGAIN. But may be someone needs to check whether the error code is a real EAGAIN and not something else, in which case the will want to use APR::Const::EAGAIN. Therefore I suppose you are right, we should expose both, and make sure that the documentation uses the IS_ version and the API doc for APR::Const::EAGAIN states that it's not cross-platform and point to APR::Status::is_EAGAIN
I know some of the constants (defines) in apr_errno.h that arise in the composites (eg, SOCEWOULDBLOCK in APR_STATUS_IS_EAGAIN) aren't supplied by APR::Const, but that should be taken care of at the C level.
right
On the other hand, simplicity is a virtue - if we wanted to start just with IS_EAGAIN, then that's fine; it's certainly easy to add others as the need arises.
:)
BTW, re: my previous comment about the name case. May be APR::Status::is_EAGAIN is better than APR::Status::IS_EAGAIN? So by using lowercase is_ we show that it's a function, but by using upcase EAGAIN we better match the error code?
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
