ronantakizawa/codereview-bench huggingface dataset:
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
 * @brief Check if the character in the pointer indicates a number.
 * It expects that you consumed all leading whitespace.
 *
 * @param p_item Pointer to verify
 * @return Non-zero integer indicating that has a digit 0 otherwise.
 */
static inline bool has_digit_int(const char *str) {
  if (!str || *str == '\0') {
    return false;
  }

  switch (*str) {
  case '0':
  case '1':
  case '2':
  case '3':
  case '4':
  case '5':
  case '6':
  case '7':
  case '8':
  case '9':
    return true;
  case '+':
  case '-':
    return str[1] != '\0' && isdigit_ascii(str[1]);
  default:
    return false;
  }
}

static inline bool has_only_spaces(const char *str) {
  while (*str != '\0' && isspace_ascii(*str)) {
    str++;
  }
  return *str == '\0';
}

/* Copy a string without `char_to_remove` into `output`,
 * it assumes that output is filled with `\0`,
 * so it won't null terminate the result.
 */
static void copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                     const char *str, char char_to_remove) {
  char *dst = output;
  const char *src = str;
  // last character is reserved for null terminator.
  const char *end = output + PROCESSED_WORD_CAPACITY - 1;

  while (*src != '\0' && dst < end) {
    const char *next = src;

Review comment (bug):
Still have an issue here where an empty string will have an address violation. This is generally why I advise passing the length of a string every time to a function - there's just a lot of edge cases

Code after:

/* Copy a string without `char_to_remove` into `output`.
 */
static int copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                    const char *str, size_t str_len,
                                    char char_to_remove) {
  const char *left = str;
  const char *end_ptr = str + str_len;
  size_t bytes_written = 0;

  while (left < end_ptr) {
    const size_t remaining_bytes_to_read = end_ptr - left;
    const char *right = memchr(left, char_to_remove, remaining_bytes_to_read);

    if (!right) {
      // If it doesn't find the char to remove, just copy until EOS.
      right = end_ptr;
    }

    const size_t chunk_size = right - left;

    if (chunk_size + bytes_written >= PROCESSED_WORD_CAPACITY) {
      return -1;
    }
    memcpy(&output[bytes_written], left, chunk_size);
    bytes_written += chunk_size;

    left = right + 1;
  }

  output[bytes_written] = '\0';
  return 0;
}

int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  const char *p = p_item;
  // Skip leading spaces.
  while (isspace_ascii(*p)) {
    ++p;
  }

  // Handle sign.
  const bool has_sign = *p == '-' || *p == '+';
  // Handle sign.
  const char *digit_start = has_sign ? p + 1 : p;

  // Check that there is a first digit.
  if (!isdigit_ascii(*digit_start)) {
    // Error...
    *error = ERROR_NO_DIGITS;
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  case '9':
    return true;
  case '+':
  case '-':
    return str[1] != '\0' && isdigit_ascii(str[1]);
  default:
    return false;
  }
}

static inline bool has_only_spaces(const char *str) {
  while (*str != '\0' && isspace_ascii(*str)) {
    str++;
  }
  return *str == '\0';
}

/* Copy a string without `char_to_remove` into `output`,
 * it assumes that output is filled with `\0`,
 * so it won't null terminate the result.
 */
static void copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                     const char *str, char char_to_remove) {
  char *dst = output;
  const char *src = str;
  // last character is reserved for null terminator.
  const char *end = output + PROCESSED_WORD_CAPACITY - 1;

  while (*src != '\0' && dst < end) {
    const char *next = src;
    // find EOS or char_to_remove
    while (*next != '\0' && *next != char_to_remove) {
      next++;
    }

    size_t len = next - src;
    if (dst + len > end) {
      // Can't write here, str is too big
      errno = ERANGE;
      return;
    }

    // copy block
    memcpy(dst, src, len);

    // go to next available location to write
    dst += len;
    // Move past char to remove
    src = *next == char_to_remove ? next + 1 : next;
  }
}

Review comment (suggestion):
There's no need to place a null terminator all the way at the end here - just place one where the string actually stops

Code after:
    if (chunk_size + bytes_written >= PROCESSED_WORD_CAPACITY) {
      return -1;
    }
    memcpy(&output[bytes_written], left, chunk_size);
    bytes_written += chunk_size;

    left = right + 1;
  }

  output[bytes_written] = '\0';
  return 0;
}

int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  const char *p = p_item;
  // Skip leading spaces.
  while (isspace_ascii(*p)) {
    ++p;
  }

  // Handle sign.
  const bool has_sign = *p == '-' || *p == '+';
  // Handle sign.
  const char *digit_start = has_sign ? p + 1 : p;

  // Check that there is a first digit.
  if (!isdigit_ascii(*digit_start)) {
    // Error...
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  char buffer[PROCESSED_WORD_CAPACITY];
  const size_t str_len = strlen(p);
  if (tsep != '\0' && memchr(p, tsep, str_len) != NULL) {
    const int status = copy_string_without_char(buffer, p, str_len, tsep);
    if (status != 0) {
      // Word is too big, probably will cause an overflow
      *error = ERROR_OVERFLOW;
      return 0;
    }
    p = buffer;
  }

  char *endptr;
  int64_t number = strtoll(p, &endptr, 10);

  if (errno == ERANGE || number > int_max || number < int_min) {
    *error = ERROR_OVERFLOW;
    errno = 0;
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  case '6':
  case '7':
  case '8':
  case '9':
    return true;
  case '+':
  case '-':
    return str[1] != '\0' && isdigit_ascii(str[1]);
  default:
    return false;
  }
}

static inline bool has_only_spaces(const char *str) {
  while (*str != '\0' && isspace_ascii(*str)) {
    str++;
  }
  return *str == '\0';
}

/* Copy a string without `char_to_remove` into `output`,
 * it assumes that output is filled with `\0`,
 * so it won't null terminate the result.
 */
static void copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                     const char *str, char char_to_remove) {
  char *dst = output;
  const char *src = str;
  // last character is reserved for null terminator.
  const char *end = output + PROCESSED_WORD_CAPACITY - 1;

  while (*src != '\0' && dst < end) {
    const char *next = src;
    // find EOS or char_to_remove
    while (*next != '\0' && *next != char_to_remove) {
      next++;
    }

    size_t len = next - src;
    if (dst + len > end) {
      // Can't write here, str is too big
      errno = ERANGE;
      return;
    }

    // copy block
    memcpy(dst, src, len);

    // go to next available location to write
    dst += len;
    // Move past char to remove

Review comment (suggestion):
I would still advise passing the length of the string as an argument here. That way you can short circuit on an issue where somehow the length of the string exceeds the buffer capacity. 

Its probably better to have a return code for this function too, rather than void

Code after:

    const size_t chunk_size = right - left;

    if (chunk_size + bytes_written >= PROCESSED_WORD_CAPACITY) {
      return -1;
    }
    memcpy(&output[bytes_written], left, chunk_size);
    bytes_written += chunk_size;

    left = right + 1;
  }

  output[bytes_written] = '\0';
  return 0;
}

int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  const char *p = p_item;
  // Skip leading spaces.
  while (isspace_ascii(*p)) {
    ++p;
  }

  // Handle sign.
  const bool has_sign = *p == '-' || *p == '+';
  // Handle sign.
  const char *digit_start = has_sign ? p + 1 : p;

  // Check that there is a first digit.
  if (!isdigit_ascii(*digit_start)) {
    // Error...
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  char buffer[PROCESSED_WORD_CAPACITY];
  const size_t str_len = strlen(p);
  if (tsep != '\0' && memchr(p, tsep, str_len) != NULL) {
    const int status = copy_string_without_char(buffer, p, str_len, tsep);
    if (status != 0) {
      // Word is too big, probably will cause an overflow
      *error = ERROR_OVERFLOW;
      return 0;
    }
    p = buffer;
  }

  char *endptr;
  int64_t number = strtoll(p, &endptr, 10);
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  }
  return *str == '\0';
}

/* Copy a string without `char_to_remove` into `output`,
 * it assumes that output is filled with `\0`,
 * so it won't null terminate the result.
 */
static void copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                     const char *str, char char_to_remove) {
  char *dst = output;
  const char *src = str;
  // last character is reserved for null terminator.
  const char *end = output + PROCESSED_WORD_CAPACITY - 1;

  while (*src != '\0' && dst < end) {
    const char *next = src;
    // find EOS or char_to_remove
    while (*next != '\0' && *next != char_to_remove) {
      next++;
    }

    size_t len = next - src;
    if (dst + len > end) {
      // Can't write here, str is too big
      errno = ERANGE;
      return;
    }

    // copy block
    memcpy(dst, src, len);

    // go to next available location to write
    dst += len;
    // Move past char to remove
    src = *next == char_to_remove ? next + 1 : next;
  }
}

int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  if (!p_item || *p_item == '\0') {
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  while (isspace_ascii(*p_item)) {
    ++p_item;
  }

  if (!has_digit_int(p_item)) {

Review comment (suggestion):
Related to my previous comment about return codes, I think in user code its better to return a code when you can rather than setting the global errno. errno is a static global, so its not thread safe, and you can be mangling codes by doing this. I think its also more of an artifact to the language than a modern error messaging solution

Code after:
int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  const char *p = p_item;
  // Skip leading spaces.
  while (isspace_ascii(*p)) {
    ++p;
  }

  // Handle sign.
  const bool has_sign = *p == '-' || *p == '+';
  // Handle sign.
  const char *digit_start = has_sign ? p + 1 : p;

  // Check that there is a first digit.
  if (!isdigit_ascii(*digit_start)) {
    // Error...
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  char buffer[PROCESSED_WORD_CAPACITY];
  const size_t str_len = strlen(p);
  if (tsep != '\0' && memchr(p, tsep, str_len) != NULL) {
    const int status = copy_string_without_char(buffer, p, str_len, tsep);
    if (status != 0) {
      // Word is too big, probably will cause an overflow
      *error = ERROR_OVERFLOW;
      return 0;
    }
    p = buffer;
  }

  char *endptr;
  int64_t number = strtoll(p, &endptr, 10);

  if (errno == ERANGE || number > int_max || number < int_min) {
    *error = ERROR_OVERFLOW;
    errno = 0;
    return 0;
  }

  // Skip trailing spaces.
  while (isspace_ascii(*endptr)) {
    ++endptr;
  }

  // Did we use up all the characters?
  if (*endptr) {
    *error = ERROR_INVALID_CHARS;
    return 0;
  }
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  if (!p_item || *p_item == '\0') {
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  while (isspace_ascii(*p_item)) {
    ++p_item;
  }

  if (*p_item == '-') {
    state->seen_sint = 1;
    *error = 0;
    return 0;
  } else if (*p_item == '+') {
    p_item++;
  }

  // Check that there is a first digit.
  if (!isdigit_ascii(*p_item)) {
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  errno = 0;
  if (tsep != '\0' && strchr(p_item, tsep) != NULL) {
    char buffer[PROCESSED_WORD_CAPACITY];
    memset(buffer, '\0', sizeof(buffer));
    copy_string_without_char(buffer, p_item, tsep);
    p_item = buffer;
  }

  if (errno == ERANGE) {
    *error = ERROR_OVERFLOW;
    return 0;
  }

  char *endptr = NULL;
  uint64_t result = strtoull(p_item, &endptr, 10);

  if (!has_only_spaces(endptr)) {
    *error = ERROR_INVALID_CHARS;
    result = 0;
  } else if (errno == ERANGE || result > uint_max) {
    *error = ERROR_OVERFLOW;
    result = 0;
  } else {
    *error = 0;
  }

  if (result > (uint64_t)int_max) {
    state->seen_uint = 1;

Review comment (bug):
I think this is undefined behavior, at least according to https://stackoverflow.com/a/79330647/621736. You may need to move the buffer declaration outside of this branch (can run ASAN/UBSAN to check)

Code after:
    }
    p = buffer;
  }

  char *endptr;
  uint64_t number = strtoull(p, &endptr, 10);

  if (errno == ERANGE || number > uint_max) {
    *error = ERROR_OVERFLOW;
    errno = 0;
    return 0;
  }

  // Skip trailing spaces.
  while (isspace_ascii(*endptr)) {
    ++endptr;
  }

  // Did we use up all the characters?
  if (*endptr) {
    *error = ERROR_INVALID_CHARS;
    return 0;
  }

  if (number > (uint64_t)int_max) {
    state->seen_uint = 1;
  }

  *error = 0;
  return number;
}
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  case '+':
  case '-':
    return str[1] != '\0' && isdigit_ascii(str[1]);
  default:
    return false;
  }
}

static inline bool has_only_spaces(const char *str) {
  while (*str != '\0' && isspace_ascii(*str)) {
    str++;
  }
  return *str == '\0';
}

/* Copy a string without `char_to_remove` into `output`.
 */
static int copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                    const char *str, size_t str_len,
                                    char char_to_remove) {
  const char *left = str;
  const char *right;
  const char *end_ptr = str + str_len;
  size_t bytes_read = 0;

  while ((right = memchr(left, char_to_remove, end_ptr - left)) != NULL) {
    size_t nbytes = right - left;

    // check if we have enough space, including the null terminator.
    if (nbytes + bytes_read >= PROCESSED_WORD_CAPACITY) {
      return ERROR_WORD2BIG;
    }
    // copy block
    memcpy(&output[bytes_read], left, nbytes);
    bytes_read += nbytes;
    left = right + 1;

    // Exit after processing the entire string
    if (left >= end_ptr) {
      break;
    }
  }

  // copy final chunk that doesn't contain char_to_remove
  if (end_ptr > left) {
    size_t nbytes = nbytes = end_ptr - left;
    if (nbytes + bytes_read >= PROCESSED_WORD_CAPACITY) {
      return ERROR_WORD2BIG;
    }
    memcpy(&output[bytes_read], left, nbytes);
    bytes_read += nbytes;

Review comment (refactor):
```suggestion
  while ((right = memchr(left, char_to_remove, str_len - bytes_read)) != NULL) {
```

Functionally the same but makes it more obvious the impacts of refactoring `left`'s usage

Code after:
    }
    memcpy(&output[bytes_written], left, chunk_size);
    bytes_written += chunk_size;

    left = right + 1;
  }

  output[bytes_written] = '\0';
  return 0;
}

int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max,
                     int *error, char tsep) {
  const char *p = p_item;
  // Skip leading spaces.
  while (isspace_ascii(*p)) {
    ++p;
  }

  // Handle sign.
  const bool has_sign = *p == '-' || *p == '+';
  // Handle sign.
  const char *digit_start = has_sign ? p + 1 : p;

  // Check that there is a first digit.
  if (!isdigit_ascii(*digit_start)) {
    // Error...
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  char buffer[PROCESSED_WORD_CAPACITY];
  const size_t str_len = strlen(p);
  if (tsep != '\0' && memchr(p, tsep, str_len) != NULL) {
    const int status = copy_string_without_char(buffer, p, str_len, tsep);
    if (status != 0) {
      // Word is too big, probably will cause an overflow
      *error = ERROR_OVERFLOW;
      return 0;
    }
    p = buffer;
  }

  char *endptr;
  int64_t number = strtoll(p, &endptr, 10);

  if (errno == ERANGE || number > int_max || number < int_min) {
    *error = ERROR_OVERFLOW;
    errno = 0;
    return 0;
  }
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  if (skip_trailing && q != NULL && *q != p) {
    while (isspace_ascii(**q)) {
      (*q)++;
    }
  }
  return r;
}

// End of xstrtod code
// ---------------------------------------------------------------------------

void uint_state_init(uint_state *self) {
  self->seen_sint = 0;
  self->seen_uint = 0;
  self->seen_null = 0;
}

int uint64_conflict(uint_state *self) {
  return self->seen_uint && (self->seen_sint || self->seen_null);
}

/**
 * @brief Check if the character in the pointer indicates a number.
 * It expects that you consumed all leading whitespace.
 *
 * @param p_item Pointer to verify
 * @return Non-zero integer indicating that has a digit 0 otherwise.
 */
static inline bool has_digit_int(const char *str) {
  if (!str || *str == '\0') {
    return false;
  }

  switch (*str) {
  case '0':
  case '1':
  case '2':
  case '3':
  case '4':
  case '5':
  case '6':
  case '7':
  case '8':
  case '9':
    return true;
  case '+':
  case '-':
    return str[1] != '\0' && isdigit_ascii(str[1]);
  default:
    return false;
  }

Review comment (suggestion):
I think for now just return -1 for error. Its a nice idea to use the define, but its inconsistent with all of the other error handling we have in our code base (and CPython standards). We don't want every function/module to implement its own custom error handling

Code after:
  PyGILState_Release(gstate);
  free(pc);
  if (skip_trailing && q != NULL && *q != p) {
    while (isspace_ascii(**q)) {
      (*q)++;
    }
  }
  return r;
}

// End of xstrtod code
// ---------------------------------------------------------------------------

void uint_state_init(uint_state *self) {
  self->seen_sint = 0;
  self->seen_uint = 0;
  self->seen_null = 0;
}

int uint64_conflict(uint_state *self) {
  return self->seen_uint && (self->seen_sint || self->seen_null);
}

/* Copy a string without `char_to_remove` into `output`.
 */
static int copy_string_without_char(char output[PROCESSED_WORD_CAPACITY],
                                    const char *str, size_t str_len,
                                    char char_to_remove) {
  const char *left = str;
  const char *end_ptr = str + str_len;
  size_t bytes_written = 0;

  while (left < end_ptr) {
    const size_t remaining_bytes_to_read = end_ptr - left;
    const char *right = memchr(left, char_to_remove, remaining_bytes_to_read);

    if (!right) {
      // If it doesn't find the char to remove, just copy until EOS.
      right = end_ptr;
    }

    const size_t chunk_size = right - left;

    if (chunk_size + bytes_written >= PROCESSED_WORD_CAPACITY) {
      return -1;
    }
    memcpy(&output[bytes_written], left, chunk_size);
    bytes_written += chunk_size;

    left = right + 1;
  }
Repository: pandas-dev/pandas
File: pandas/_libs/src/parser/tokenizer.c (C)

Code before:
  char buffer[PROCESSED_WORD_CAPACITY];
  if (tsep != '\0' && strchr(p_item, tsep) != NULL) {
    int status = copy_string_without_char(buffer, p_item, strlen(p_item), tsep);

    if (status != 0) {
      *error = status;
      return 0;
    }

    p_item = buffer;
  }

  if (errno == ERANGE) {
    *error = ERROR_OVERFLOW;
    return 0;
  }

  char *endptr = NULL;
  int64_t result = strtoll(p_item, &endptr, 10);

  if (!has_only_spaces(endptr)) {
    // Check first for invalid characters because we may
    // want to skip integer parsing if we find one.
    *error = ERROR_INVALID_CHARS;
    result = 0;
  } else if (errno == ERANGE || result > int_max || result < int_min) {
    *error = ERROR_OVERFLOW;
    result = 0;
  } else {
    *error = 0;
  }

  return result;
}

uint64_t str_to_uint64(uint_state *state, const char *p_item, int64_t int_max,
                       uint64_t uint_max, int *error, char tsep) {
  if (!p_item || *p_item == '\0') {
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  while (isspace_ascii(*p_item)) {
    ++p_item;
  }

  if (*p_item == '-') {
    state->seen_sint = 1;
    *error = 0;
    return 0;
  } else if (*p_item == '+') {

Review comment (performance):
I suppose this is unrelated to your PR but why does this function accept `int_max` and `int_min` as arguments? Assuming those are actually set to `INT64_MAX` and `INT64_MIN` this is a no-op, and potentially waste of cycles if the compiler can't optimize it away

Code after:
  // Handle sign.
  if (*p == '-') {
    state->seen_sint = 1;
    *error = 0;
    return 0;
  } else if (*p == '+') {
    p++;
  }

  // Check that there is a first digit.
  if (!isdigit_ascii(*p)) {
    // Error...
    *error = ERROR_NO_DIGITS;
    return 0;
  }

  char buffer[PROCESSED_WORD_CAPACITY];
  const size_t str_len = strlen(p);
  if (tsep != '\0' && memchr(p, tsep, str_len) != NULL) {
    const int status = copy_string_without_char(buffer, p, str_len, tsep);
    if (status != 0) {
      // Word is too big, probably will cause an overflow
      *error = ERROR_OVERFLOW;
      return 0;
    }
    p = buffer;
  }

  char *endptr;
  uint64_t number = strtoull(p, &endptr, 10);

  if (errno == ERANGE || number > uint_max) {
    *error = ERROR_OVERFLOW;
    errno = 0;
    return 0;
  }

  // Skip trailing spaces.
  while (isspace_ascii(*endptr)) {
    ++endptr;
  }

  // Did we use up all the characters?
  if (*endptr) {
    *error = ERROR_INVALID_CHARS;
    return 0;
  }

  if (number > (uint64_t)int_max) {
    state->seen_uint = 1;
  }