Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Switch more size_t references to int64_t #20786

Merged
merged 1 commit into from Apr 24, 2018
Merged

BUG: Switch more size_t references to int64_t #20786

merged 1 commit into from Apr 24, 2018
+18 −17

Conversation

ginggs
Copy link
Contributor

@ginggs ginggs commented Apr 22, 2018

closes #20785

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@ginggs
Copy link
Contributor Author

ginggs commented Apr 22, 2018

While I agree with @jeffknupp's comment in #17063 that we should really be using size_t for these types, this patch switches some size_t that were switched from int in d5c75e8 and not subsequently switched to int64_t in e04d12a or 3171674

@ginggs
Copy link
Contributor Author

ginggs commented Apr 22, 2018

This patch seems to fix the issue on 32-bit big-endian, I'm running some tests now to make sure it doesn't break anywhere else.

jreback
jreback requested changes Apr 22, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

this looks fine. can u add a whatsnew note (u can put in bug fix io section)

@codecov
Copy link

codecov bot commented Apr 22, 2018 •
edited

Codecov Report

Merging #20786 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #20786   +/-   ##
=======================================
  Coverage   91.84%   91.84%           
=======================================
  Files         153      153           
  Lines       49305    49305           
=======================================
  Hits        45286    45286           
  Misses       4019     4019
Flag Coverage Δ
#multiple 90.24% <ø> (ø) ⬆️
#single 41.89% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4de2e9b...96308eb. Read the comment docs.

@jreback jreback added IO CSV read_csv, to_csv Compat pandas objects compatability with Numpy or Python functions 32bit 32-bit systems labels Apr 22, 2018
@ginggs ginggs changed the title WIP: Switch more size_t references to int64_t BUG: Switch more size_t references to int64_t Apr 22, 2018
@ginggs
Copy link
Contributor Author

ginggs commented Apr 22, 2018

All tests OK on this side, and I added a whatsnew note.

@jreback jreback added this to the 0.23.0 milestone Apr 22, 2018
@jreback
Copy link
Contributor

jreback commented Apr 22, 2018

look good. ping on green.

@ginggs
Copy link
Contributor Author

ginggs commented Apr 22, 2018

ping

@jreback jreback merged commit 0c27f40 into pandas-dev:master Apr 24, 2018
@jreback
Copy link
Contributor

jreback commented Apr 24, 2018

thanks @ginggs

@ginggs ginggs deleted the more-int64_t branch April 24, 2018 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
32bit 32-bit systems Compat pandas objects compatability with Numpy or Python functions IO CSV read_csv, to_csv
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

heap corruption in read_csv on 32-bit big-endian architectures
2 participants
@ginggs @jreback