Fix fread segfault from faulty assumption on compilers

[badasahog]

Closed #7183 cc @MichaelChirico

[github-actions]

• HEAD=segfaultfix slower P<0.001 for setDT improved in #5427
  

Generated via commit e850ac1

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

Task	Duration
R setup and installing dependencies	2 minutes and 38 seconds
Installing different package versions	19 seconds
Running and plotting the test cases	2 minutes and 36 seconds

[MichaelChirico]

thanks! would you mind

(1) adding a helpful comment for any intrepid future devs that have the same thoughts about making it const
(2) re-read the older PR again with fresh eyes, if memory serves this type of change was made more than once [it could just mean the other instances are not covered by tests & this not showing up]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (c8bbb58) to head (e850ac1).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7200   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          81       81           
  Lines       15223    15223           
=======================================
  Hits        15037    15037           
  Misses        186      186           

☔ 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]

I believe these are also at risk:

data.table/src/fread.c

Line 775 in c8bbb58

const bool Eneg = ch[1] == '-';

data.table/src/fread.c

Line 946 in c8bbb58

const bool Eneg = ch[1] == '-';

Can you think of an input file that would trigger the segfault? If so we should add them as regression tests.

[badasahog]

Those wouldn't create a problem because ch[1] is tested first anyway (I'm not rearranging the order of access as far as I can tell)

[MichaelChirico]

Oh, got it, ch[0] is already implicitly tested before in both cases:

data.table/src/fread.c

Line 772 in c8bbb58

if (*ch == 'E' || *ch == 'e') {

data.table/src/fread.c

Line 932 in c8bbb58

if (ch[0] == '0' && (ch[1] == 'x' || ch[1] == 'X') &&

[MichaelChirico]

I suppose we could retain the const label by adding a layer of nesting:

if (ch[0] == '0' && (ch[1] == 'x' || ch[1] == 'X')) {
  const bool subnormal = ch[2] == '0';
  if ((subnormal || ch[2] == '1') && ch[3] == '.') {
    // ...
  }
}

But I'm not sure the const offers that much benefit to be worth the slightly worse readability of the code.

[MichaelChirico]

Thanks!

[badasahog]

the whole point of adding const is to improve readability and clarify intent. adding would seem counterintuitive.