On Thursday, July 25, 2013, Stephen Frost wrote:
On Wednesday, July 24, 2013, Stephen Frost wrote:
Unfortunately, I don't have the code in front of me at the moment, but I was pretty sure FlushErrorState cleans up the intermediate information set up during an individual error call and doesn't completely clear the stack info (tho I can't think of what, if anything, does..) or multiple "raise notices" wouldn't each contain the stack info in their output..
I'll certainly look through this again and dream up some additional test cases for it, in the morning.
Couldn't help if- reading code on a phone isn't ideal, but that's a different discussion.
You're right- resetting the stack depth doesn't make any sense here, which FlushErrorState does. I think clearing the memory context should be alright but that's a different story. Curious that the raise notice in the regression test didn't blow up, which is part of why I thought it was fine, but a subsequent call to raise notice in the same function would be a good test (along with another call to this function..) and I think wouldn't produce a stack trace here, which would be quite wrong.
Will address once I'm back I front of something I can actually write code on.
Alright, no, and clearly I should have run this down before committing it, but I knew the raise notice after the call to get diag meant we must not be completely destroying the stack.
The errdata stack is different from the context stack info. errdata is for reentrant error calls, which this function should never be a part of. Were that to happen, at least the Assert around the recursion check should fire. As for if we *should* call FlushErrorState, I think we need to, to reset ErrorContext and move the errdata stack depth back to -1, where it should be. Now, we might just forgo adding ourselves onto the stack, as we don't set a callback anyway, but the errdata stack depth should certainly be at -1 when we leave, or we haven't prevented reentrant calls into GetErrorStack.
This definitely deserves more commentary and I'll see about adding that also.
Thanks for reviewing!
Stephen