Skip to content

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

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 7 commits into from
Jun 22, 2019
Merged

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac9536d
BLD: use unsigned instead of signed for lengths, avoid build warnings
jbrockmendel Jun 10, 2019
6f90c8e
Merge branch 'master' of https://github.com/pandas-dev/pandas into to…
jbrockmendel Jun 10, 2019
d61082b
edit for comment and lint
jbrockmendel Jun 10, 2019
37b6002
Merge branch 'master' of https://github.com/pandas-dev/pandas into to…
jbrockmendel Jun 11, 2019
f29bfdc
remove casts
jbrockmendel Jun 11, 2019
15c1c5d
Merge branch 'master' of https://github.com/pandas-dev/pandas into to…
jbrockmendel Jun 21, 2019
fa66545
update to unsigned types
jbrockmendel Jun 21, 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
18 changes: 9 additions & 9 deletions pandas/_libs/parsers.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,24 +119,24 @@ cdef extern from "parser/tokenizer.h":

# where to write out tokenized data
char *stream
int64_t stream_len
int64_t stream_cap
uint64_t stream_len
uint64_t stream_cap

# Store words in (potentially ragged) matrix for now, hmm
char **words
int64_t *word_starts # where we are in the stream
int64_t words_len
int64_t words_cap
int64_t max_words_cap # maximum word cap encountered
uint64_t words_len
uint64_t words_cap
uint64_t max_words_cap # maximum word cap encountered

char *pword_start # pointer to stream start of current field
int64_t word_start # position start of current field

int64_t *line_start # position in words for start of line
int64_t *line_fields # Number of fields in each line
int64_t lines # Number of lines observed
int64_t file_lines # Number of lines observed (with bad/skipped)
int64_t lines_cap # Vector capacity
uint64_t lines # Number of lines observed
uint64_t file_lines # Number of lines observed (with bad/skipped)
uint64_t lines_cap # Vector capacity

# Tokenizing stuff
ParserState state
Expand Down Expand Up @@ -168,7 +168,7 @@ cdef extern from "parser/tokenizer.h":

int header # Boolean: 1: has header, 0: no header
int64_t header_start # header row start
int64_t header_end # header row end
uint64_t header_end # header row end

void *skipset
PyObject *skipfunc
Expand Down
23 changes: 15 additions & 8 deletions pandas/_libs/src/parser/tokenizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ void parser_del(parser_t *self) {
}

static int make_stream_space(parser_t *self, size_t nbytes) {
int64_t i, cap, length;
uint64_t i, cap;
int64_t length;
Copy link
Member

Choose a reason for hiding this comment

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

Does leaving this as is not introduce any new build warnings? Looks like assignments and comparisons are done with unsigned types within this function

Copy link
Contributor

Choose a reason for hiding this comment

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

why is length signed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there weren't any warnings telling us not to. Probably makes more sense unsigned liked the others.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this, otherwise lgtm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this requires changing a few other places to match. I think its benign, but not obvious. Go for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep prob should

int status;
void *orig_ptr, *newptr;

Expand Down Expand Up @@ -471,7 +472,7 @@ static int end_line(parser_t *self) {
return 0;
}

if (!(self->lines <= (int64_t) self->header_end + 1) &&
if (!(self->lines <= self->header_end + 1) &&
(self->expected_fields < 0 && fields > ex_fields) && !(self->usecols)) {
// increment file line count
self->file_lines++;
Expand Down Expand Up @@ -507,7 +508,7 @@ static int end_line(parser_t *self) {
}
} else {
// missing trailing delimiters
if ((self->lines >= (int64_t) self->header_end + 1) &&
if ((self->lines >= self->header_end + 1) &&
fields < ex_fields) {
// might overrun the buffer when closing fields
if (make_stream_space(self, ex_fields - fields) < 0) {
Expand Down Expand Up @@ -651,7 +652,7 @@ static int parser_buffer_bytes(parser_t *self, size_t nbytes) {
stream = self->stream + self->stream_len; \
slen = self->stream_len; \
self->state = STATE; \
if (line_limit > 0 && self->lines == start_lines + (int64_t)line_limit) { \
if (line_limit > 0 && self->lines == start_lines + (uint64_t)line_limit) { \
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to even have these casts? Fraught with peril to begin with using macros I think this just adds nuances / developer burden

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to remove these casts.

Should we consider getting rid of the macros? That's outside my region of competence.

Copy link
Member

Choose a reason for hiding this comment

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

Yea might be worth doing so in a separate PR. Here's the cpp take on macros:

https://isocpp.org/wiki/faq/inline-functions#inline-vs-macros

I could see this one being particular troublesome with our codebase:

https://isocpp.org/wiki/faq/misc-technical-issues#macros-with-if

I think the main downside to using inline vs macros would be support for pre-C99 compilers, but I don't think that's a problem given age and the fact that we have inline functions elsewhere in the code base

goto linelimit; \
}

Expand All @@ -666,7 +667,7 @@ static int parser_buffer_bytes(parser_t *self, size_t nbytes) {
stream = self->stream + self->stream_len; \
slen = self->stream_len; \
self->state = STATE; \
if (line_limit > 0 && self->lines == start_lines + (int64_t)line_limit) { \
if (line_limit > 0 && self->lines == start_lines + (uint64_t)line_limit) { \
goto linelimit; \
}

Expand Down Expand Up @@ -737,7 +738,8 @@ int skip_this_line(parser_t *self, int64_t rownum) {

int tokenize_bytes(parser_t *self,
size_t line_limit, int64_t start_lines) {
int64_t i, slen;
int64_t i;
uint64_t slen;
int should_skip;
char c;
char *stream;
Expand Down Expand Up @@ -1203,7 +1205,8 @@ static int parser_handle_eof(parser_t *self) {
}

int parser_consume_rows(parser_t *self, size_t nrows) {
int64_t i, offset, word_deletions, char_count;
int64_t offset, word_deletions;
uint64_t char_count, i;

if (nrows > self->lines) {
nrows = self->lines;
Expand All @@ -1229,6 +1232,8 @@ int parser_consume_rows(parser_t *self, size_t nrows) {
self->stream_len -= char_count;

/* move token metadata */
// Note: We should always have words_len < word_deletions, so this
// subtraction will remain appropriately-typed.
for (i = 0; i < self->words_len - word_deletions; ++i) {
offset = i + word_deletions;

Expand All @@ -1242,6 +1247,8 @@ int parser_consume_rows(parser_t *self, size_t nrows) {
self->word_start -= char_count;

/* move line metadata */
// Note: We should always have self->lines - nrows + 1 >= 0, so this
// subtraction will remain appropriately-typed.
for (i = 0; i < self->lines - nrows + 1; ++i) {
offset = i + nrows;
self->line_start[i] = self->line_start[offset] - word_deletions;
Expand All @@ -1265,7 +1272,7 @@ int parser_trim_buffers(parser_t *self) {
size_t new_cap;
void *newptr;

int64_t i;
uint64_t i;

/**
* Before we free up space and trim, we should
Expand Down
18 changes: 9 additions & 9 deletions pandas/_libs/src/parser/tokenizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,24 @@ typedef struct parser_t {

// where to write out tokenized data
char *stream;
int64_t stream_len;
int64_t stream_cap;
uint64_t stream_len;
uint64_t stream_cap;

// Store words in (potentially ragged) matrix for now, hmm
char **words;
int64_t *word_starts; // where we are in the stream
int64_t words_len;
int64_t words_cap;
int64_t max_words_cap; // maximum word cap encountered
uint64_t words_len;
uint64_t words_cap;
uint64_t max_words_cap; // maximum word cap encountered

char *pword_start; // pointer to stream start of current field
int64_t word_start; // position start of current field

int64_t *line_start; // position in words for start of line
int64_t *line_fields; // Number of fields in each line
int64_t lines; // Number of (good) lines observed
int64_t file_lines; // Number of lines (including bad or skipped)
int64_t lines_cap; // Vector capacity
uint64_t lines; // Number of (good) lines observed
uint64_t file_lines; // Number of lines (including bad or skipped)
uint64_t lines_cap; // Vector capacity

// Tokenizing stuff
ParserState state;
Expand Down Expand Up @@ -153,7 +153,7 @@ typedef struct parser_t {

int header; // Boolean: 1: has header, 0: no header
int64_t header_start; // header row start
int64_t header_end; // header row end
uint64_t header_end; // header row end

void *skipset;
PyObject *skipfunc;
Expand Down