-
Notifications
You must be signed in to change notification settings - Fork 1k
Use _RO accessors for const targets #7611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7611 +/- ##
==========================================
- Coverage 99.02% 99.02% -0.01%
==========================================
Files 87 87
Lines 16903 16890 -13
==========================================
- Hits 16739 16726 -13
Misses 164 164 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Generated via commit 5fe9198 Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
Another big class of cases where we could switch to There are roughly 240 such sites ( |
|
I see this has some (or total) overlap with #7394. I think it's good to break out the changes into minimally-digestible parts -- here we focus only on |
|
Should we add an accessor for #define INTEGER64_RO(x) ((const int64_t *) DATAPTR_RO(x)) |
Good point, though I think out of scope -- #7618 |
Co-authored-by: Benjamin Schwendinger <[email protected]>
| if (!isInteger(idx)) internal_error(__func__, "Argument '%s' to %s is type '%s' not '%s'", "idx", "check_idx", type2char(TYPEOF(idx)), "integer"); // # nocov | ||
| bool anyLess=false, anyNA=false; | ||
| int last = INT32_MIN; | ||
| int *idxp = INTEGER(idx), n=LENGTH(idx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const int *idxp = INTEGER_RO(idx), n=LENGTH(idx); |
| COMPARE1 COMPARE2 | ||
| } | ||
| } else { | ||
| const double *vd=(const double *)REAL(v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const double *vd=REAL_RO(v); |
| const R_xlen_t n = xlength(x); | ||
| if (n==0) | ||
| return ScalarInteger(0); // empty vector | ||
| int first = LOGICAL(x)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR is about smth else but just spotted this :/
should be bool. same applies for second and third
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's right -- see below first==NA_INTEGER, that's not possible with bool.
Maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! But then still should be NA_LOGICAL. Also only cosmetics since R defines like this
#define NA_LOGICAL R_NaInt
#define NA_INTEGER R_NaInt| int thislen = 0; | ||
| if (data->narm) { | ||
| SEXP thisidx = VECTOR_ELT(data->not_NA_indices, j); | ||
| ithisidx = INTEGER(thisidx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ithisidx = INTEGER_RO(thisidx); |
| case RAWSXP: { Rbyte *ansd=RAW(tmp); SHIFT(Rbyte, RAW, ansd[ansi++]=val); } break; | ||
| case LGLSXP: { int *ansd=LOGICAL(tmp); SHIFT(int, LOGICAL, ansd[ansi++]=val); } break; | ||
| case INTSXP: { int *ansd=INTEGER(tmp); SHIFT(int, INTEGER, ansd[ansi++]=val); } break; | ||
| case REALSXP: { double *ansd=REAL(tmp); SHIFT(double, REAL, ansd[ansi++]=val); } break; | ||
| // integer64 is shifted as if it's REAL; and assigning fill=NA_INTEGER64 is ok as REAL | ||
| case CPLXSXP: { Rcomplex *ansd=COMPLEX(tmp); SHIFT(Rcomplex, COMPLEX, ansd[ansi++]=val); } break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| case RAWSXP: { Rbyte *ansd=RAW(tmp); SHIFT(Rbyte, RAW_RO, ansd[ansi++]=val); } break; | |
| case LGLSXP: { int *ansd=LOGICAL(tmp); SHIFT(int, LOGICAL_RO, ansd[ansi++]=val); } break; | |
| case INTSXP: { int *ansd=INTEGER(tmp); SHIFT(int, INTEGER_RO, ansd[ansi++]=val); } break; | |
| case REALSXP: { double *ansd=REAL(tmp); SHIFT(double, REAL_RO, ansd[ansi++]=val); } break; | |
| // integer64 is shifted as if it's REAL; and assigning fill=NA_INTEGER64 is ok as REAL | |
| case CPLXSXP: { Rcomplex *ansd=COMPLEX(tmp); SHIFT(Rcomplex, COMPLEX_RO, ansd[ansi++]=val); } break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But only cosmetic if we dont remove the (const CTYPE *) cast
|
This Coccinelle spatch should at least help as a linter:
@@
type T;
const T *variable;
expression E;
@@
- variable = REAL(E)
+ variable = REAL_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = INTEGER(E)
+ variable = INTEGER_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = COMPLEX(E)
+ variable = COMPLEX_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = RAW(E)
+ variable = RAW_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = LOGICAL(E)
+ variable = LOGICAL_RO(E)
|

Searched the sources:
grep -Er "const\b.*[*]\w+\s*=\s*[A-Z]+([^OR]|[^A]R)[(]" srcThis is some low-hanging fruit in terms of better use of read-only accessors. There will be other cases where we can make the change to a
constpointer as well, but those can't be identified with a simple regex.