Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix some C++ warnings#10383
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
Fix some C++ warnings#10383
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -109,8 +109,13 @@ int convert_double(PyObject *obj, void *p) | |||
| int convert_bool(PyObject *obj, void *p) | |||
| { | |||
| bool *val = (bool *)p; | |||
| int ret; | |||
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.
just change the signature of convert_bool toint convert_bool(PyObject *obj, bool *p)?
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.
All of the convert functions usevoid*.
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.
not convinced that they should but I guess that's another project...
| @@ -387,7 +392,7 @@ int convert_path(PyObject *obj, void *pathp) | |||
| if (should_simplify_obj == NULL) { | |||
| goto exit; | |||
| } | |||
| should_simplify = PyObject_IsTrue(should_simplify_obj); | |||
| should_simplify = PyObject_IsTrue(should_simplify_obj) != 0; | |||
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.
perhaps use convert_bool? dunno
src/_backend_agg_wrapper.cpp Outdated
| @@ -430,7 +430,7 @@ static PyObject *PyRendererAgg_draw_quad_mesh(PyRendererAgg *self, PyObject *arg | |||
| offsets, | |||
| offset_trans, | |||
| facecolors, | |||
| antialiased, | |||
| antialiased != 0, | |||
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.
wouldn't it be simpler (throughout) to make antialiased a bool?
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.
Done, mostly.
src/_backend_agg.h Outdated
| @@ -1008,7 +1008,7 @@ inline void RendererAgg::_draw_path_collection_generic(GCAgg &gc, | |||
| gc.isaa = antialiaseds(i % Naa); | |||
| transformed_path_t tpath(path, trans); | |||
| nan_removed_t nan_removed(tpath, true, has_curves); | |||
| nan_removed_t nan_removed(tpath, true, has_curves != 0); | |||
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.
make has_curves a bool?
src/_contour.cpp Outdated
| case Edge_N: return BOUNDARY_N(quad_edge.quad); | ||
| case Edge_W: return BOUNDARY_W(quad_edge.quad); | ||
| case Edge_S: return BOUNDARY_S(quad_edge.quad); | ||
| case Edge_E: return BOUNDARY_E(quad_edge.quad) != 0; |
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.
modify the macro accordingly?
(same comment applies more or less throughout)
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.
Fixed.
src/_image_wrapper.cpp Outdated
| "norm :float, optional\n" | ||
| "The norm for the interpolation function. Default is 0.\n\n" | ||
| "norm :bool, optional\n" | ||
| "Whether to norm the interpolation function. Default is 0.\n\n" |
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.
default is false
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.
Fixed.
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.
few more comments left to be addressed (make macros return bools, etc) but already an improvement.
cd50cd3 tofa5e2a3CompareTest failure doesn't look spurious. |
It is; that one fails sporadically due to some filename re-use. I've been meaning to look into it... |
Do we want to hold off on this until we get a 2.2 rc tagged? |
I don't think it needs to wait--but your asking gives me pause on clicking the button now... |
BTW, in Python 3.3, |
btw do you really have a warning on implicitly casting a number to a bool? (dunno, just asking) |
Can merge now? |
setupext.py Outdated
| ] | ||
| ext = make_extension('matplotlib.ft2font', sources) | ||
| FreeType().add_flags(ext) | ||
| Numpy().add_flags(ext) | ||
| LibAgg().add_flags(ext) |
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.
so libagg became a "dep" of ft2font via py_converters? probably not a big deal but yuck... now that we're py3 only we may as well use thep converter flag instead of convert_bool, if that avoids the additional internal dep.
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.
py_converters has Agg stuff in it so includes its headers, and then we need to add its flags... I think I can split them up so that this isn't necessary.
Usingp will still produce anint which caused the warnings on AppVeyor.
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.
Though maybe I'd wait for#10426 so I don't have to worry about the old backends.
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.
but we can just do the cast of the result ofp ourselves?
I don't want to force you to overengineer this, the addition of agg as a dependency only somewhat annoys me.
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.
But then we'd need all the!= 0 that you didn't like, no? BecauseMSVC didn't like(bool)var either...
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.
if an explicit typecast triggers a warning that seems a bit too much but can't do anything about it :/ I guess that sort of forces towards using!= 0 (or!!x) then...
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.
If it helps you feel a little better, I also stopped it adding Agg sources to these extensions, which was unnecessary.
This prevents a bunch of repeated warnings since these headers areincluded multiple times.
This prevents a warning casting from double to float if the variable isnot double.
It only accepts a boolean on the Python side, but this is not enforcedon the C++ side. This causes many casting warnings since it's usedmultiple times calling various filters that only take a `bool`.
No need to cast the int before dividing, because the divisor is adouble. It is necessary to cast the entire expression though, since thestorage variable is only a float.
This helps fix up some bool warnings.
Rebased and tweaked setup a little so Agg sources don't get built into the new ones. |
good to go once appveyor passes |
PR Summary
This fixes various minor type conversion and other casting warnings. It is not completely clean now, but saves about 100 lines of warnings in every AppVeyor build.
PR Checklist