bpo-45635: standardize error handling in traceback.c#29905
bpo-45635: standardize error handling in traceback.c#29905iritkatriel merged 6 commits intopython:mainfrom
Conversation
Python/traceback.c
Outdated
| if (err != 0) { | ||
| return err; | ||
| } | ||
| if (err == -1) |
There was a problem hiding this comment.
Nitpick: I prefer the if (err < 0) C idiom; but it is also what the majority of the code base uses for error handling.
Python/traceback.c
Outdated
| done: | ||
| return err; |
There was a problem hiding this comment.
Did you consider using this "CPython idiom" throughout the PR (ditching err, where possible)?
if (fn() < 0) {
goto error;
}
return 0;
error:
return -1;
}
IMHO, the advantage is that you've got two clear exit paths; I don't have to backtrack err to find out if the function will return 0 or -1 for each bad/good path. Probably just personal preference :)
There was a problem hiding this comment.
In this case you're right, but there are places where there is some XDECREF that needs to happen regardless of the return value.
There was a problem hiding this comment.
I guess it could be
Py_XDECREF(thing);
return 0;
error:
Py_XDECREF(thing);
return -1;
}
There was a problem hiding this comment.
True, in some situations, it may not be the best alternative because of ref counting; if you need a lot of decrefs before each goto, it is probably not worth it.
There was a problem hiding this comment.
Or something like this, if possible.
int ret = fn(obj);
Py_DECREF(obj);
if (ret < 0) {
goto error;
}
return 0;
error:
return -1;
There was a problem hiding this comment.
So I did this everywhere and I agree it's much better now.
The only place where things are a little weird is tb_displayline, because there are cases where there is an early exit with return value 0 (depending on the return value of some "should I ignore this error" function).
Python/traceback.c
Outdated
| error: | ||
| return -1; |
There was a problem hiding this comment.
We can drop the error label, and just return -1 iso. goto error.
Python/traceback.c
Outdated
|
|
||
| if (err == 0) { | ||
| err = _Py_WriteIndentedMargin(margin_indent, margin, f); | ||
| assert(!err); |
There was a problem hiding this comment.
err is set twice: first at initialisation (set to 0), and then set to -1 if linjeobj == NULL in the get-lineno-loop. If lineobj == NULL, we bail after closing fob and cleaning up refs. I think it would be an improvement to remove err and simply return -1 in line 523. Then we'd also get rid of this slightly confusing (to me) assert :)
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland
left a comment
erlend-aasland
left a comment
There was a problem hiding this comment.
Looks good!
OT (but semi-related):
I've only been able to squeeze out ~56% region coverage and ~44% branch coverage of traceback.c with test_traceback test_warnings test_raise test_pyexpat. Running the whole test suite gives me ~62% region coverage and ~48% branch coverage, but I've not been able to find out where those last tests are hidden 🔍
IMO, we should...:
- improve region and branch coverage to at least 80%
- consolidate
traceback.ctests intest_tracebackto make it easier to detect regressions and new bugs
I think I got 85% line coverage on a mac a few days ago (on main). I don't think this branch reduced it by much. |
Wow. |
lcov. Just the standard thing from the devguide. |
👍🏻 But what region/branch coverage did you get? |
It doesn't say, just 85% of the lines and 100% of the functions. |
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit a05ac63 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Those two tests are failing on other branches as well. |
4 participants
https://bugs.python.org/issue45635