Make path extension a bit safer #30208
Conversation
|
|
||
| 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.
I don't think the compiler can safely elide the bounds check here, because it'll have trouble proving that size is small enough (I guess the "modern C++" way of ensuring that is to make NUM_VERTICES an int templated on code etc.)
There was a problem hiding this comment.
Well, x.at is the bounds-checked version, and x[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?)
|
Instead of the tuple, I thought it better to use the |
A secondary reason is it makes extension to 3D a bit simpler, as we can eventually template on |
... by replacing double pointers by fixed-size `std::array`, or a return `tuple`. With gcc (and optimization enabled?), this has no effect on code size, but gives compile-time (and better runtime) checks that there are 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 compiler warns about a possible ODR violation (which isn't a problem since it's only used in one file.)
By using the existing `XY` type to replace x/y pairs, and taking advantage of struct methods.
Use `XY` type to shorten internals, and `agg::rect_d::normalize` to shorten initialization.
|
I just noticed that 6d5dd9a 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.
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.
get_path_collection_extents always starts with a reset so it should start with max/min infinity. And then update_path_extents calls .update after a PathNanRemover. So we should never have NaN in either.
anntzer
left a comment
There was a problem hiding this comment.
Approval modulo one question re: interactions with nans.
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