Skip to content

Fix build warnings #27157

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from Jul 2, 2019
Merged

Fix build warnings #27157

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d41edd5
CLN: fix build warning
pilkibun Jun 30, 2019
160b55d
CLN: fix build warning
pilkibun Jun 30, 2019
a6a252a
CLN: fix build warning
pilkibun Jun 30, 2019
125c97a
CLN: fix build warning
pilkibun Jun 30, 2019
0931a1a
CLN: fix build warning
pilkibun Jun 30, 2019
50f819b
CLN: fix build warning
pilkibun Jun 30, 2019
ca7d77c
CLN: fix build warning
pilkibun Jun 30, 2019
75537d9
CLN: fix build warning
pilkibun Jul 1, 2019
168462d
CLN: fix build warning
pilkibun Jul 1, 2019
913956d
CLN: fix build warning
pilkibun Jul 1, 2019
6554425
CLN: fix build warning
pilkibun Jul 1, 2019
7e9117b
CLN: fix build warning
pilkibun Jul 1, 2019
d2440ef
Revert "CLN: fix build warning"
pilkibun Jul 1, 2019
08b9606
CLN: fix build warning
pilkibun Jul 1, 2019
9db4c05
CLN: fix test
pilkibun Jul 1, 2019
d914206
CEC8
pilkibun Jul 1, 2019
71c7d62
CLN: use snprintf not strncpy to silence both compiler and cpplint
pilkibun Jul 1, 2019
d2e8f66
Use Py_ssize_t not ssize_t
pilkibun Jul 1, 2019
ac4c0b3
omit template changes
pilkibun Jul 1, 2019
832d55d
Retry CI
pilkibun Jul 1, 2019
790a8e7
use Py_ssize_t
pilkibun Jul 1, 2019
71b4df0
remove redundant sizeof
pilkibun Jul 1, 2019
e464800
Use memory views without specific memory layout
pilkibun Jul 1, 2019
d8f3fa5
BUG: check for empty values in index.pyx/_bin_search
pilkibun Jul 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pandas/_libs/index.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ cdef Py_ssize_t _bin_search(ndarray values, object val) except -1:
Py_ssize_t mid = 0, lo = 0, hi = len(values) - 1
Copy link
Author

@ghost ghost Jul 1, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real bug I think. mid is referenced in the return value if len(values)=1 so that the main loop immediately falls through.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this would fail if len(values) == 0 right? Do you see a code sample to actually trigger that?

Copy link
Author

@ghost ghost Jul 1, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite possibly this is meant to be "caller checks". But that's out of the compiler's view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea sure. Similar comment though - good to suppress compiler warning but if there's an actual error here (which I think would still happen without the warning) would be good to address

Copy link
Author

@ghost ghost Jul 1, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if values is empty, the loop falls through, util.get_value_at(values, mid) gets called, which calls validate_indexer which raises. I think it's ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which raises

cython code can have weird corner cases where exceptions get ignored. Adding a test for this case seems worthwhile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. example?

Copy link
Author

@ghost ghost Jul 1, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh. I think you've nailed something here. So it's not exactly weird corner cases, but cdef functions which return exception as if they were python functions.
https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values

validate_indexer and get_value_at are both cdef. validate_indexer is tagged except -1, so any exception translates into a -1 retval. But the caller get_value_at doesn't check that and uses -1 to reference into the the array. in this case an empty one. but it doesn't have an except return value defines. So, I'm not sure what happens.

If an exception is detected in such a function, a warning message is 
printed and the exception is ignored.

OTOH, the index constructor refuses to create empty indexes, so an end-to-end test does not seem possible. and get_value_at is used all over the place.

Hmm. will have to think about this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In [18]: %%cython
    ...: 
    ...: cdef int foo() except -1:
    ...:     1/0
    ...:     cdef:
    ...:         int mid = 42
    ...:     return mid
    ...:     
    ...: cdef int bar():
    ...:     res = foo()
    ...:     return res
    ...:     
    ...: def baz():
    ...:     return bar()

In [19]: baz()
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
_cython_magic_a591cd36cd8e49116d6e370949ee24c2.pyx in _cython_magic_a591cd36cd8e49116d6e370949ee24c2.foo()

ZeroDivisionError: float division
Exception ignored in: '_cython_magic_a591cd36cd8e49116d6e370949ee24c2.bar'
Traceback (most recent call last):
  File "_cython_magic_a591cd36cd8e49116d6e370949ee24c2.pyx", line 3, in _cython_magic_a591cd36cd8e49116d6e370949ee24c2.foo
ZeroDivisionError: float division
Out[19]: 0

very interesting bug.

object pval

if hi >= 0 and val > util.get_value_at(values, hi):
if hi == 0 or (hi > 0 and val > util.get_value_at(values, hi)):
return len(values)

while lo < hi:
Expand Down