Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
Make path extension a bit safer#30208
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| for (size_t i = 1; i < size; ++i) { | ||
| unsigned subcode = path.vertex(&x[i], &y[i]); | ||
| unsigned subcode = path.vertex(&x.at(i), &y.at(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think the compiler can safely elide the bounds check here, because it'll have trouble proving thatsize is small enough (I guess the "modern C++" way of ensuring that is to make NUM_VERTICES an int templated oncode etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well,x.at is the bounds-checked version, andx[i] isn't, but somehow the compiled code remains the same size either way. (Perhaps this is because the Fedora compiler has hardening enabled somewhere?)
Uh oh!
There was an error while loading.Please reload this page.
QuLogic commentedSep 11, 2025
Instead of the tuple, I thought it better to use the |
QuLogic commentedSep 12, 2025
A secondary reason is it makes extension to 3D a bit simpler, as we can eventually template on |
scottshambaugh left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks cleaner & safer to me, no changes stand out
... by replacing double pointers by fixed-size `std::array`, or a return`tuple`. With gcc (and optimization enabled?), this has no effect oncode size, but gives compile-time (and better runtime) checks that thereare no out-of-bounds access.
It is `bool` for the Python wrapper, while internally `int`, but can be`bool` consistently.Also mark it as `inline` since it's used in a template and the compilerwarns about a possible ODR violation (which isn't a problem since it'sonly used in one file.)
By using the existing `XY` type to replace x/y pairs, and takingadvantage of struct methods.
Use `XY` type to shorten internals, and `agg::rect_d::normalize` toshorten initialization.
QuLogic commentedSep 16, 2025
I just noticed that6d5dd9a was empty, so I'll rebase that out. |
| void update(double x, double y) | ||
| { | ||
| start.x = std::min(start.x, x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is the new form equivalent to the old one if either start.x or x is nan? (Ditto for all other similar changes.)
(It's also possible that the answer is "we never have nans here and it doesn't matter", I haven't checked.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
get_path_collection_extents always starts with areset so it should start with max/min infinity. And thenupdate_path_extents calls.update after aPathNanRemover. So we should never have NaN in either.
anntzer left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approval modulo one question re: interactions with nans.
74beb8d intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
PR summary
By replacing double pointers by
std::arrayand returned tuples. AFAICT, this doesn't have any effect on code size, but ensures that several places are checked at compile time. And for now, we already know these to be correct, but this would prevent any future problems if some sizes change.PR checklist