Repository navigation
Make path extension a bit safer - #30208
anntzer merged 4 commits into
Conversation
Sorry, something went wrong.
|
|
||
| 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.)
Sorry, something went wrong.
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?)
Sorry, something went wrong.
|
Instead of the tuple, I thought it better to use the XY type we already have here. Also tacked on changing to that for the extent limits struct as well. |
Sorry, something went wrong.
A secondary reason is it makes extension to 3D a bit simpler, as we can eventually template on XY and (to be PR'd) XYZ somewhat straightforwardly. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks cleaner & safer to me, no changes stand out
Sorry, something went wrong.
... 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. |
Sorry, something went wrong.
|
|
||
| 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.)
Sorry, something went wrong.
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.
Sorry, something went wrong.
anntzer
left a comment
There was a problem hiding this comment.
Approval modulo one question re: interactions with nans.
Sorry, something went wrong.
PR summary
By replacing double pointers by std::array and 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