You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Alex Karasulu <ao...@bellsouth.net> on 2004/04/04 21:14:41 UTC
[digester] pop(), peek() methods don't need to catch exceptions
Hi,
I was just looking at the digester code as I was writing another incarnation
of the digester pattern and noticed the pop() and peek() methods do not need
to catch exceptions. It is just cheaper to check the size of the stack
before the pop() or peek() calls and return null instead of just making that
call surrounded by a catch block.
You guys want me to just make and commit these changes? It's not really a
big deal but thought I'd let you guys know about it.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Alex Karasulu <ao...@bellsouth.net>.
> I think having pop throw an EmptyStackException is much nicer, even if
> it is inconsistent with the current Digester.pop/peek.
+1
--Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Wed, 2004-04-07 at 05:20, Craig R. McClanahan wrote:
> robert burrell donkin wrote:
>
> > On 5 Apr 2004, at 03:27, Simon Kitching wrote:
> >
> > <snip>
> >
> >> You will see in CVS that Robert's new "named stack" pop method does
> >> throw an EmptyStackException when an empty stack is popped, and I think
> >> that is the correct behaviour. If we do a digester 2.0, I would
> >> recommend that the pop() method throw an exception in this case - unless
> >> I've missed a good reason for the null to be returned. I can't think of
> >> one, though.
> >
> >
> > IIRC i preferred the version that returned null but was persuaded that
> > throwing an exception was more consistent with the sun collections API
> > and was therefore what users would probably expect. had i check the
> > original API and discovered that the rest of digester returned nulls,
> > i would probably have changed my mind again. so, i suppose that i
> > don't have a strong opinion on what's better :)
>
> How about going with "to thine own self be true" and copy what we did on
> the standard object stack?
I had a look at all the places where Digester.pop() is called. In all
cases, popping an empty stack is wrong, and *should* result in an error
being reported. I expect that the same applies to Digester.peek().
Unfortunately because of the "null returned on empty stack" solution,
what will happen in most of these cases is a NullPointerException at
some random time after the real bug (the bad pop) instead of an
EmptyStackException at the exact point that the bug occurred.
I think having pop throw an EmptyStackException is much nicer, even if
it is inconsistent with the current Digester.pop/peek.
Regards,
Simon
>
> :-)
>
> Craig
>
> >
> > - robert
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by "Craig R. McClanahan" <cr...@apache.org>.
robert burrell donkin wrote:
> On 5 Apr 2004, at 03:27, Simon Kitching wrote:
>
> <snip>
>
>> You will see in CVS that Robert's new "named stack" pop method does
>> throw an EmptyStackException when an empty stack is popped, and I think
>> that is the correct behaviour. If we do a digester 2.0, I would
>> recommend that the pop() method throw an exception in this case - unless
>> I've missed a good reason for the null to be returned. I can't think of
>> one, though.
>
>
> IIRC i preferred the version that returned null but was persuaded that
> throwing an exception was more consistent with the sun collections API
> and was therefore what users would probably expect. had i check the
> original API and discovered that the rest of digester returned nulls,
> i would probably have changed my mind again. so, i suppose that i
> don't have a strong opinion on what's better :)
How about going with "to thine own self be true" and copy what we did on
the standard object stack?
:-)
Craig
>
> - robert
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On 5 Apr 2004, at 03:27, Simon Kitching wrote:
<snip>
> You will see in CVS that Robert's new "named stack" pop method does
> throw an EmptyStackException when an empty stack is popped, and I think
> that is the correct behaviour. If we do a digester 2.0, I would
> recommend that the pop() method throw an exception in this case -
> unless
> I've missed a good reason for the null to be returned. I can't think of
> one, though.
IIRC i preferred the version that returned null but was persuaded that
throwing an exception was more consistent with the sun collections API
and was therefore what users would probably expect. had i check the
original API and discovered that the rest of digester returned nulls, i
would probably have changed my mind again. so, i suppose that i don't
have a strong opinion on what's better :)
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Alex Karasulu <ao...@bellsouth.net>.
Simon,
> if (stack.isEmpty()) {
> return null;
> } else {
> // right here some other thread could empty the stack, causing
> // the following pop to throw an exception despite our
> // check above
> return stack.pop();
> }
Yep gotcha now and I'm glad to hear that the pop and peek methods will
throw the EmptyStackException. I did think it was weird that they did
not however I figured there was something I was missing. I was trying to
think about whether or not I should adopt the same behavior in the
Digester pattern implementation (not for XML) I'm currently working on.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Mon, 2004-04-05 at 14:13, Alex Karasulu wrote:
> You really don't know how many times the empty stack is going to have pop or
> peek called by a client. So this really is not a situation that we know is
> only going happen once and a while. "Cheaper" comes from the fact that it
> costs let to check if the stack is empty than it does to create an exception
> object, fill its stack trace, and hurl it out the door to have it caught
> again.
>
> However it certainly is a good point that the conditional test for empty
> will always occur. Making the change really is not a big deal I thought I'd
> just point it out. We can just leave it as is.
I would expect that anywhere in Digester, popping an empty object stack
is (or should be) regarded as an error which immediately terminates all
processing. I don't know why this method was ever coded to return null
when an empty stack is popped. So the situation you describe, in which
an Exception object is created and filled in, should never happen in a
successful parse, and only once in an unsuccessful parse.
You will see in CVS that Robert's new "named stack" pop method does
throw an EmptyStackException when an empty stack is popped, and I think
that is the correct behaviour. If we do a digester 2.0, I would
recommend that the pop() method throw an exception in this case - unless
I've missed a good reason for the null to be returned. I can't think of
one, though.
> > Of course the current exception-based approach is thread-safe, while an
> > if-based approach is not, though that is not an issue here.
>
> Please excuse me for being dense but how is throwing the exception a thread
> safe situation? Before the exception is thrown does the stack not check the
> same variable to see if it is empty or not?
Yes, but if the stack implementation is a thread-safe stack, then the
exception-based approach will also be thread-safe. However the if-based
approach is *always* vulnerable to a race condition if some other thread
can change the stack size between the if and the pop:
if (stack.isEmpty()) {
return null;
} else {
// right here some other thread could empty the stack, causing
// the following pop to throw an exception despite our
// check above
return stack.pop();
}
Cheers,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Alex Karasulu <ao...@bellsouth.net>.
> -----Original Message-----
> From: Simon Kitching [mailto:simon@ecnetwork.co.nz]
> > I was just looking at the digester code as I was writing another
> incarnation
> > of the digester pattern and noticed the pop() and peek() methods do not
> need
> > to catch exceptions. It is just cheaper to check the size of the stack
> > before the pop() or peek() calls and return null instead of just making
> that
> > call surrounded by a catch block.
> >
> > You guys want me to just make and commit these changes? It's not really
> a
> > big deal but thought I'd let you guys know about it.
>
> I don't mind. I don't see any harm, but don't see much benefit either.
>
> What is the performance of a try/catch clause when exceptions aren't
> thrown, vs an "if (stack.isEmpty())" which is *always* executed? Is it
> truly "cheaper" to use the if?
You really don't know how many times the empty stack is going to have pop or
peek called by a client. So this really is not a situation that we know is
only going happen once and a while. "Cheaper" comes from the fact that it
costs let to check if the stack is empty than it does to create an exception
object, fill its stack trace, and hurl it out the door to have it caught
again.
However it certainly is a good point that the conditional test for empty
will always occur. Making the change really is not a big deal I thought I'd
just point it out. We can just leave it as is.
> Of course the current exception-based approach is thread-safe, while an
> if-based approach is not, though that is not an issue here.
Please excuse me for being dense but how is throwing the exception a thread
safe situation? Before the exception is thrown does the stack not check the
same variable to see if it is empty or not?
--Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Mon, 2004-04-05 at 07:14, Alex Karasulu wrote:
> Hi,
>
> I was just looking at the digester code as I was writing another incarnation
> of the digester pattern and noticed the pop() and peek() methods do not need
> to catch exceptions. It is just cheaper to check the size of the stack
> before the pop() or peek() calls and return null instead of just making that
> call surrounded by a catch block.
>
> You guys want me to just make and commit these changes? It's not really a
> big deal but thought I'd let you guys know about it.
I don't mind. I don't see any harm, but don't see much benefit either.
What is the performance of a try/catch clause when exceptions aren't
thrown, vs an "if (stack.isEmpty())" which is *always* executed? Is it
truly "cheaper" to use the if?
Of course the current exception-based approach is thread-safe, while an
if-based approach is not, though that is not an issue here.
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On 4 Apr 2004, at 20:14, Alex Karasulu wrote:
> Hi,
>
> I was just looking at the digester code as I was writing another
> incarnation
> of the digester pattern and noticed the pop() and peek() methods do
> not need
> to catch exceptions. It is just cheaper to check the size of the stack
> before the pop() or peek() calls and return null instead of just
> making that
> call surrounded by a catch block.
sounds reasonable to me. anyone else see any reason not to make these
changes?
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: [digester] pop(), peek() methods don't need to catch exceptions
Posted by Alex Karasulu <ao...@bellsouth.net>.
> +0. No problems, but in the big scheme of things I suspect it won't
> make any substantive difference in performance.
NP, I figured as much. I just see these sorts of things and they bother
me like imports that are not used. I just thought it would be the right
thing to ask before doing something about it. Otherwise I would not
bother conversing over such petty details.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] pop(), peek() methods don't need to catch exceptions
Posted by "Craig R. McClanahan" <cr...@apache.org>.
Alex Karasulu wrote:
>Hi,
>
>I was just looking at the digester code as I was writing another incarnation
>of the digester pattern and noticed the pop() and peek() methods do not need
>to catch exceptions. It is just cheaper to check the size of the stack
>before the pop() or peek() calls and return null instead of just making that
>call surrounded by a catch block.
>
>You guys want me to just make and commit these changes? It's not really a
>big deal but thought I'd let you guys know about it.
>
>
+0. No problems, but in the big scheme of things I suspect it won't
make any substantive difference in performance.
>Alex
>
>
Craig
>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org