Use read-only accessors for more call sites

[MichaelChirico]

These call sites were identified by gemini; it identified the file name & line number of possible sites, and I reviewed and applied the edits. I instructed it to ignore cases like those mentioned in #7611 (comment) -- they are quite numerous and of questionable value; anyway, they are easy to find with regex. The goal here was to comb the source for places that require a bit more contextualization to classify.

Currently based against #7611.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (60f2ba2) to head (2b330f3).
⚠️ Report is 1 commits behind head on ro-access.

Additional details and impacted files

@@            Coverage Diff             @@
##           ro-access    #7615   +/-   ##
==========================================
  Coverage      99.02%   99.02%           
==========================================
  Files             87       87           
  Lines          16903    16903           
==========================================
  Hits           16739    16739           
  Misses           164      164           

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

[github-actions]

No obvious timing issues in HEAD=ro-part-ii

Generated via commit 2b330f3

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

Task	Duration
R setup and installing dependencies	5 minutes and 23 seconds
Installing different package versions	11 minutes and 37 seconds
Running and plotting the test cases	3 minutes and 40 seconds

[ben-schwen]

Went through all of them and LGTM.

But shouldn't the compiler cry anyway when we write at *_RO vectors since it it basically a write at const int?

[MichaelChirico]

Went through all of them and LGTM.

But shouldn't the compiler cry anyway when we write at *_RO vectors since it it basically a write at const int?

I agree the compiler should complain if we get one wrong

[MichaelChirico]

(oops @ben-schwen this was targeting the other PR branch, #7611, not master 😬)

[MichaelChirico]

But shouldn't the compiler cry anyway when we write at *_RO vectors since it it basically a write at const int?

Yes... I suppose the maximalist way to fix this would be to just replace INTEGER with INTEGER_RO everywhere, try to compile, iterate. It would wind up including the 100s cases like LOGICAL(narm)[0], generating some code bloat by subverting the very common idiom. I only see 20 files on CRAN doing that (funnily enough, two are ours 😅)

https://github.com/search?q=org%3Acran%20lang%3Ac%20%2FINTEGER_RO%5C(%5Cw%2B%5C)%5C%5B%5Cw%2B%5C%5D%2F&type=code

data.table/src/types.c

Line 76 in 596f65d

int istatus = INTEGER_RO(status)[0], nx = INTEGER_RO(x)[0], nk = INTEGER_RO(k)[0];

Another thing to explore could be changing up so that the default is read-only, and defining our own macro for writeable accessors:

• INTEGER(): current INTEGER_RO()
• INTEGER_W() - current INTEGER()

But that makes our codebase confusing for outside contributors.

[ben-schwen]

(oops @ben-schwen this was targeting the other PR branch, #7611, not master 😬)

I just saw it 🙈
Should I revert?

[MichaelChirico]

I think it's fine to squash+merge both to master, just makes reviewing the other one a little more tedious.

Would you mind reviewing it? It'll be easiest for you since you already reviewed this one.