Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make path extension a bit safer#30208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| double last_x = 0.0; | ||
| double last_y = 0.0; | ||
| unsigned code; | ||
| while ((code = path.vertex(&x[0], &y[0])) != agg::path_cmd_stop) { | ||
| while ((code = path.vertex(&std::get<0>(x), &std::get<0>(y))) != agg::path_cmd_stop) { |
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 think you can still do&x[0] no? (orx.at(0) if youreally want bounds checking here; this still reads better than std::get I'd say)
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.
std::get is compile-time checked for constants; neitherx[0] norx.at(0) are unfortunately.
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.
Ugh, indeed, that's a bit annoying...
| if (code == CLOSEPOLY) { | ||
| buffer +=codes[4]; | ||
| buffer +=std::get<4>(codes); |
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.
ditto
| } else if (code < 5) { | ||
| size_t size = NUM_VERTICES[code]; | ||
| 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?)
src/_path_wrapper.cpp Outdated
| @@ -252,16 +252,11 @@ static py::object | |||
| Py_convert_to_string(mpl::PathIterator path, agg::trans_affine trans, | |||
| agg::rect_d cliprect, std::optional<bool> simplify, | |||
| SketchParams sketch,int precision, | |||
| std::array<std::string,5>codes_obj,bool postfix) | |||
| std::array<std::string,5>codes,bool postfix) | |||
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 think this could beconst std::array<std::string, 5> &codes with corresponding changes toconvert_to_string and__convert_to_string?
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.
Yes, indeed, and it even saves 1.5% code (which to be fair is only 4K, so maybe only a page.)
... 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.
... by avoiding double pointers.
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