removed redundant object declarations

[badasahog]

Followup to #7138

I wanted to separate this into a different pr because I have a feeling this code was here intentionally (although I can't tell why).

I'm 100% sure it doesn't do anything.

This pr also has a failing test, but I've verified that that test also fails on the current master.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (c8bbb58) to head (5476c96).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7181      +/-   ##
==========================================
- Coverage   98.77%   98.77%   -0.01%     
==========================================
  Files          81       81              
  Lines       15223    15212      -11     
==========================================
- Hits        15037    15026      -11     
  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.

[aitap]

Would you like to try your hand at fixing the failing test? ERROR: AddressSanitizer: heap-buffer-overflow on address 0x521002ab4501 at pc 0x79e50be8e21a bp 0x7ffeff19acd0 sp 0x7ffeff19acc8 READ of size 1 at 0x521002ab4501 thread T0 #0 0x79e50be8e219 in parse_double_hexadecimal /builds/Rdatatable/data.table/data.table.Rcheck/00_pkg_src/data.table/src/fread.c:930:26 https://github.com/Rdatatable/data.table/blame/0a11ffad762d4d2f8f367a07f04fe52a0a3777ac/src/fread.c#L930

[github-actions]

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

Generated via commit 5476c96

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

Task	Duration
R setup and installing dependencies	2 minutes and 46 seconds
Installing different package versions	40 seconds
Running and plotting the test cases	2 minutes and 37 seconds

[badasahog]

That's not the test I was referring to.

Running test id 2334         Test 2334 ran without errors but failed check that x equals y:
> x = fread(f)
             V1  [Key= Types=dou Classes=num]
    1: 1.999985
    2: 1.999985
   ---
19999: 1.999985
20000: 1.999985
> y = TRUE
First 1 of 1 (type 'logical'):
[1] TRUE

[aitap]

That's not the test 2334 on current master or in this PR: https://github.com/Rdatatable/data.table/blob/bd936a06ec148ceb9d16091a8ac1cb1f659b04e2/inst/tests/tests.Rraw#L21537 Your branch seems fine. Perhaps you need to rebuild the package tarball using 'R CMD build' before running 'R CMD check' on it?

[badasahog]

I'm on Windows. Really I'm not sure how to address this because I'm not familiar with the production process.

If it's not in my branch then is this pr ready to go?

[MichaelChirico]

Here are the logs of the error:

https://gitlab.com/Rdatatable/data.table/-/jobs/10696837289

I think we should fix the bug (presumably introduced by #7138) before continuing to dig a hole with further edits to the file.

[MichaelChirico]

BTW, from the history, I guess these local variables were chosen as a way to minimize the diff in this rather large PR: 05edd24

Since before, all those variables were declared in the signature, assigning them locally by reading ctx avoided needing to make all the edits you've done in the same PR & ballooning the reviewer burden.

[jangorecki]

Since before, all those variables were declared in the signature, assigning them locally by reading ctx avoided needing to make all the edits you've done in the same PR & ballooning the reviewer burden.

Many edits suffers readability of the code.
What this change is trying solve?

[badasahog]

Many edits suffers readability of the code.
What this change is trying solve?

huh?

[jangorecki]

This change makes code less readable, not sure what are the benefits here

[jangorecki]

Seems like object declaration was useful to make code easier to read, so not really redundant.

But as I don't do any fread C coding I leave it up to other devs who maintain that part.

[badasahog]

I think it makes it better, as its clear where the data is coming from. This has all the downsides of an abstraction while also making the function longer.

[MichaelChirico]

I am OK w the change, my only question is, is there no benefit to the const annotation to signal that these objects are read-only within the scope & therefore ctx won't be written from this routine?

[badasahog]

The compiler can figure that out