iframe-proxy

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

 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

        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

                                      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


            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

-        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

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

        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

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

QuLogic commented Feb 6, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift commented Feb 6, 2018

Uh oh!

QuLogic commented Feb 6, 2018

Uh oh!

tacaswell commented Feb 7, 2018

Uh oh!

dopplershift commented Feb 8, 2018

Uh oh!

QuLogic commented Feb 12, 2018

Uh oh!

anntzer commented Feb 13, 2018

Uh oh!

QuLogic commented Feb 13, 2018

Uh oh!

QuLogic commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Feb 17, 2018

Uh oh!

anntzer commented Feb 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anntzer Feb 6, 2018 •

edited

Loading