removed const qualifier

[badasahog]

I removed the const qualifier, as well as the const_cast function.

Since it was a read-mostly object, the performance hit should be minimal, while removing all undefined behavior.

We can also experiment with restrict qualified pointers, but I want to get this merged first to avoid a longer approval process.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (6f54ccd) to head (30a1917).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7037      +/-   ##
==========================================
- Coverage   98.69%   98.69%   -0.01%     
==========================================
  Files          79       79              
  Lines       14676    14673       -3     
==========================================
- Hits        14485    14482       -3     
  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.

[badasahog]

I don't know what's wrong with the benchmark system, I had the same problem on my other PRs

[MichaelChirico]

I don't know what's wrong with the benchmark system, I had the same problem on my other PRs

looks like something transient maybe...

[MichaelChirico]

the performance hit should be minimal

It would be good to quantify this, exactly. Are we talking % difference, fraction-of-%? Let's see if the atime suite tells us anything; would there be an easy way to measure it more directly, too?

I'm particularly looking at the first case in the args.filename branch, which is very common.

[github-actions]

No obvious timing issues in HEAD=constCastRemoval

Generated via commit 30a1917

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	4 minutes and 56 seconds
Installing different package versions	9 minutes and 28 seconds
Running and plotting the test cases	2 minutes and 3 seconds

[aitap]

Let's try to get rid of the newly appearing compiler warnings:

fread.c: In function ‘freadMain’:
fread.c:1471:9: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 1471 |     sof = (const char*) mmp;

fread.c:1543:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 1543 |         eof = lastNewLine;

[badasahog]

It's like christmas lights. If I untangle these warnings the entire thing falls apart.

Why do we even need the cow page? I'm not sure I understand its purpose.

[aitap]

There's code in fread.c written to the invariant that *eof=='\0'. If it instead treated 'eof' as a past-the-end pointer that should never be dereferenced (and the buffer as non-0-terminated), the code wouldn't need to create that last zero in the memory in the first place.

[badasahog]

@aitap I can't find that. can you point me to it?

If so, that could be a silver bullet.

[aitap]

It's not just one place, the code has been assuming this for 8 years. Consider eol(): if there's not a \0 at the end and the buffer consists of ASCII carriage returns, it'll be overrun. Consider the callers of eol: sometimes they check ch == eof after dereferencing *ch. It'll take a very careful reading of the 2064 lines of code to untangle.