DM-19690: audit for ast error-checking #52
Conversation
Added assertOK() where it had been left out. This is C code, so it needs to be done after every astXXX function call, except for a few (e.g., astAnnul) and where it's in a destructor. Otherwise, the ast global error state can be set, we don't realise it, and the error appears to come out of somewhere else.
| "%s", options.c_str()))) {} | ||
| "%s", options.c_str()))) { | ||
| assertOK(); | ||
| } |
There was a problem hiding this comment.
This is redundant because the Frame constructor calls the Mapping constructor which calls assertOK, and after that is called no AST code is executed that can set the flag. I admit that astIsA<x> is often called in these constructors, but:
- I don't think that can set the OK flag false
- Even if it can, this would only happen if it returned false, and at that point it's too late to call assertOK as an exception is already being raised
Where was the missing assertOK that triggered this bug? I think almost all the ones you added are redundant.
There was a problem hiding this comment.
I have not followed the code paths, but I put assertOK calls after any ast* calls (except for astIsA*). I figure it's better to be more careful than less careful, because it's a cheap test and if anything is missed, it manifests as a completely weird bug, and takes a full day of mucking around with gdb to understand it.
There was a problem hiding this comment.
That's a good answer. OK. Go ahead.
Added assertOK() where it had been left out. This is C code, so
it needs to be done after every astXXX function call, except for
a few (e.g., astAnnul) and where it's in a destructor. Otherwise,
the ast global error state can be set, we don't realise it, and
the error appears to come out of somewhere else.