BLD: fix inline warnings #17528
BLD: fix inline warnings #17528
Conversation
|
perf tests, just to confirm this doesn't change anything. don't have to run them all, maybe a subset esp indexing. |
jreback
added
the
Compat
|
Test error looks unrelated to the PR. Is this correct? |
|
|
Test failure looks unrelated. Pls confirm. |
|
so the asv is too flaky, pls run again. Its very important that this doesn't change much for this PR> |
|
I'm not comfortable with the heuristic "let's cherry-pick the best ASV run". My understanding is that removing the "inline" annotations should do nothing but remove the warnings at build time, but my understanding is not especially reliable in this area. If we aren't collectively confident in our ability to reason about this code, then a) we shouldn't be touching it, and b) we have a bigger problem than annoying warnings in the build log. |
|
In the previous PR, you referenced a cython discussion (cython/cython#1706 (comment)) which gives a nice summary. Assuming that is correct, I think we can 'reason' about this PR (athough a less flaky asv to confirm our reasoning would still be good) Eg taking (note I am not very familiar with this, I just tried to understand the comment in cython/cython#1706 (comment)) |
Codecov Report
@@ Coverage Diff @@
## master #17528 +/- ##
==========================================
+ Coverage 91.19% 91.64% +0.44%
==========================================
Files 163 163
Lines 49627 52400 +2773
==========================================
+ Hits 45259 48020 +2761
- Misses 4368 4380 +12
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17528 +/- ##
==========================================
- Coverage 91.22% 91.2% -0.02%
==========================================
Files 163 163
Lines 49655 49655
==========================================
- Hits 45297 45288 -9
- Misses 4358 4367 +9
Continue to review full report at Codecov.
|
asv runs are generally quite stable on the same machine commit-to-commit. I find this a pretty good tool to find performance regressions. A lot of the cython code is designed to address performance issues, so using a tool for that is generally a good thing. So your comments here are a bit odd. |
|
in any event pls rebase |
|
couple more as an aside, if you are looking for something to do..... These warnings have been on the build forever. I think you can just define it to remove, maybe need to change some amount of code (#8329). |
jbrockmendel
force-pushed
the
inline_cleanup
branch
from
1bd7936 to
0f565fc
Compare
Excellent. Will push shortly.
Long todo list as it is... but that warning has bugged me, so I'll probably take a swing at this anyway.
Sidenote: window.pyx has That path looks wrong to me. Is the ".." not one level too many?
That line is Which is fixed in a commit about to be pushed. But it makes me think that the version that util.pxd externs (what's the appropriate verb here?) from numpy_helper probably a) is unneeded and b) doesn't exist. |
|
After some googling and trial+error on the NPY_NO_DEPRECATED_API topic, I have found a new way to cause compile-time errors. This is all FYI, not relevant to this PR. Readers of The Future, feel free to skip. In setup.py, passing Based on this, I'm questioning the wording of the warning message. |
| @@ -255,7 +255,7 @@ cdef extern from "parser/tokenizer.h": | |||
|
|
|||
| # inline int to_complex(char *item, double *p_real, | |||
| # double *p_imag, char sci, char decimal) | |||
| inline int to_longlong(char *item, long long *p_value) nogil | |||
There was a problem hiding this comment.
actually this line doesn't appear to be used anymore, so I think can comment out
There was a problem hiding this comment.
OK. I've got an upcoming PR that this will be appropriate for. Is there a pattern for when to comment out vs delete?
There was a problem hiding this comment.
these should all be deleted
|
thanks, this is good |
Previous PR: #17277