Skip to content

Conversation

@badasahog
Copy link
Contributor

@badasahog badasahog commented May 12, 2025

In many places, there was an explicit cast to size_t, but this is clunky and unnecessary because the sizeof operator always evaluates to type size_t

https://godbolt.org/z/x77oKPsE7

@badasahog badasahog requested a review from aitap as a code owner May 12, 2025 06:36
@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (17160aa) to head (f1b99b9).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6959   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          79       79           
  Lines       14685    14685           
=======================================
  Hits        14494    14494           
  Misses        191      191           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MichaelChirico
Copy link
Member

LGTM, is this something Coccinelle can help with @aitap?

@aitap
Copy link
Member

aitap commented May 13, 2025

@MichaelChirico, surprisingly, no, not this time. Coccinelle doesn't handle the macros in fread.c well enough and ends up skipping the lines we want to edit/check while recovering from parse errors.

Some of these casts to (int) could well be integer overflows on a 64-bit system:

STOP(_("Failed to allocate %d bytes for '%s': %s"), (int)(sizeof(*size) * (size_t)ncol), "size", strerror(errno)); // # nocov

STOP(_("Failed to allocate %d bytes for '%s'."), (int)(ndropFill * sizeof(int)), "dropFill"); // # nocov

But then again, we also have integer overflows on the non-error paths in fread.c and segfaults reading long files because of those.

@aitap aitap merged commit 3815402 into Rdatatable:master May 13, 2025
10 of 11 checks passed
@aitap
Copy link
Member

aitap commented May 29, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants