perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stas Bekman <s...@stason.org>
Subject Re: adding APR::Status?
Date Tue, 19 Apr 2005 14:25:36 GMT
Randy Kobes wrote:
> 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:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org


Mime
View raw message