Fix some C++ warnings #10383

QuLogic · 2018-02-06T01:04:12Z

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

Has Pytest style unit tests
Code is PEP 8 compliant
[N/A] New features are documented, with examples if plot related
[N/A] Documentation is sphinx and numpydoc compliant
[N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
[N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer · 2018-02-06T07:56:11Z

src/py_converters.cpp

@@ -109,8 +109,13 @@ int convert_double(PyObject *obj, void *p)
 int convert_bool(PyObject *obj, void *p)
 {
    bool *val = (bool *)p;
+    int ret;


just change the signature of convert_bool to int convert_bool(PyObject *obj, bool *p)?

All of the convert functions use void*.

not convinced that they should but I guess that's another project...

anntzer · 2018-02-06T07:56:52Z

src/py_converters.cpp

@@ -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;


perhaps use convert_bool? dunno

anntzer · 2018-02-06T08:01:09Z

src/_backend_agg_wrapper.cpp

@@ -430,7 +430,7 @@ static PyObject *PyRendererAgg_draw_quad_mesh(PyRendererAgg *self, PyObject *arg
                                      offsets,
                                      offset_trans,
                                      facecolors,
-                                      antialiased,
+                                      antialiased != 0,


wouldn't it be simpler (throughout) to make antialiased a bool?

Done, mostly.

anntzer · 2018-02-06T08:06:48Z

src/_backend_agg.h

@@ -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);


make has_curves a bool?

anntzer · 2018-02-06T08:07:35Z

src/_contour.cpp

-        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;


modify the macro accordingly?
(same comment applies more or less throughout)

anntzer · 2018-02-06T08:54:09Z

src/_image_wrapper.cpp

-"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"


default is false

anntzer

few more comments left to be addressed (make macros return bools, etc) but already an improvement.

dopplershift · 2018-02-06T20:04:44Z

Test failure doesn't look spurious.

QuLogic · 2018-02-06T20:22:40Z

It is; that one fails sporadically due to some filename re-use. I've been meaning to look into it...

tacaswell · 2018-02-07T20:32:42Z

Do we want to hold off on this until we get a 2.2 rc tagged?

dopplershift · 2018-02-08T22:37:59Z

I don't think it needs to wait--but your asking gives me pause on clicking the button now...

QuLogic · 2018-02-12T05:30:37Z

BTW, in Python 3.3, PyArg_ParseTupleAndKeywords now supports a p type which returns a boolean value as an int. However, then we'd have to do a != 0 thing to avoid the warnings again. Not sure if that's better or worse than the converter function we have now.

anntzer · 2018-02-13T21:15:47Z

btw do you really have a warning on implicitly casting a number to a bool? (dunno, just asking)

QuLogic · 2018-02-13T21:30:12Z

AppVeyor does, e.g., https://ci.appveyor.com/project/matplotlib/matplotlib/build/1.0.6311/job/f886au7ulbu70hlj#L798

QuLogic · 2018-02-15T01:33:41Z

Can merge now?

anntzer · 2018-02-15T02:20:57Z

setupext.py

            ]
        ext = make_extension('matplotlib.ft2font', sources)
        FreeType().add_flags(ext)
        Numpy().add_flags(ext)
+        LibAgg().add_flags(ext)


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 the p converter flag instead of convert_bool, if that avoids the additional internal dep.

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.

Using p will still produce an int which caused the warnings on AppVeyor.

Though maybe I'd wait for #10426 so I don't have to worry about the old backends.

but we can just do the cast of the result of p ourselves?
I don't want to force you to overengineer this, the addition of agg as a dependency only somewhat annoys me.

But then we'd need all the != 0 that you didn't like, no? Because MSVC didn't like (bool)var either...

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...

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 are included multiple times.

This prevents a warning casting from double to float if the variable is not double.

It only accepts a boolean on the Python side, but this is not enforced on the C++ side. This causes many casting warnings since it's used multiple times calling various filters that only take a `bool`.

No need to cast the int before dividing, because the divisor is a double. It is necessary to cast the entire expression though, since the storage variable is only a float.

This helps fix up some bool warnings.

QuLogic · 2018-02-17T02:06:25Z

Rebased and tweaked setup a little so Agg sources don't get built into the new ones.

anntzer · 2018-02-17T05:28:52Z

good to go once appveyor passes

anntzer reviewed Feb 6, 2018

View reviewed changes

QuLogic force-pushed the c-warnings branch from 8e3d72c to d684c52 Compare February 6, 2018 08:49

anntzer reviewed Feb 6, 2018

View reviewed changes

anntzer approved these changes Feb 6, 2018

View reviewed changes

QuLogic force-pushed the c-warnings branch 3 times, most recently from cd50cd3 to fa5e2a3 Compare February 6, 2018 09:30

tacaswell added this to the v3.0 milestone Feb 7, 2018

dopplershift approved these changes Feb 8, 2018

View reviewed changes

QuLogic mentioned this pull request Feb 13, 2018

Make the filternorm prop of Images a boolean rather than a {0,1} scalar. #10442

Merged

6 tasks

anntzer reviewed Feb 15, 2018

View reviewed changes

QuLogic mentioned this pull request Feb 17, 2018

Remove Python 2 code from C extensions #10507

Merged

1 task

QuLogic added 6 commits February 16, 2018 21:03

Remove unused variable.

429ffce

Add explicit casts in some headers.

881f2a6

This prevents a bunch of repeated warnings since these headers are included multiple times.

Use float constants where needed.

a57ba96

This prevents a warning casting from double to float if the variable is not double.

Make norm argument to _image.resample a bool.

ff4ab66

It only accepts a boolean on the Python side, but this is not enforced on the C++ side. This causes many casting warnings since it's used multiple times calling various filters that only take a `bool`.

Use correct types for array sizes.

bd746cd

Fix casts that apply to whole expression.

3c130cb

No need to cast the int before dividing, because the divisor is a double. It is necessary to cast the entire expression though, since the storage variable is only a float.

QuLogic added 2 commits February 16, 2018 21:03

Make some C++ bool conversions explicit.

23a8b00

Use convert_bool in more places.

abd5942

This helps fix up some bool warnings.

QuLogic force-pushed the c-warnings branch from fa5e2a3 to abd5942 Compare February 17, 2018 02:04

anntzer merged commit 96c2876 into matplotlib:master Feb 17, 2018

QuLogic deleted the c-warnings branch February 17, 2018 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some C++ warnings #10383

Fix some C++ warnings #10383

QuLogic commented Feb 6, 2018

anntzer Feb 6, 2018

QuLogic Feb 6, 2018

anntzer Feb 6, 2018

anntzer Feb 6, 2018

anntzer Feb 6, 2018

QuLogic Feb 6, 2018

anntzer Feb 6, 2018

anntzer Feb 6, 2018 •

edited

Loading

QuLogic Feb 6, 2018

anntzer Feb 6, 2018

QuLogic Feb 6, 2018

anntzer left a comment

dopplershift commented Feb 6, 2018

QuLogic commented Feb 6, 2018

tacaswell commented Feb 7, 2018

dopplershift commented Feb 8, 2018

QuLogic commented Feb 12, 2018

anntzer commented Feb 13, 2018

QuLogic commented Feb 13, 2018

QuLogic commented Feb 15, 2018

anntzer Feb 15, 2018

QuLogic Feb 15, 2018

QuLogic Feb 15, 2018

anntzer Feb 15, 2018

QuLogic Feb 15, 2018

anntzer Feb 15, 2018

QuLogic Feb 17, 2018

QuLogic commented Feb 17, 2018

anntzer commented Feb 17, 2018

Fix some C++ warnings #10383

Fix some C++ warnings #10383

Conversation

QuLogic commented Feb 6, 2018

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

dopplershift commented Feb 6, 2018

QuLogic commented Feb 6, 2018

tacaswell commented Feb 7, 2018

dopplershift commented Feb 8, 2018

QuLogic commented Feb 12, 2018

anntzer commented Feb 13, 2018

QuLogic commented Feb 13, 2018

QuLogic commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic commented Feb 17, 2018

anntzer commented Feb 17, 2018

anntzer Feb 6, 2018 •

edited

Loading