Skip to content

Commit f9c2824

Browse files
authored
Avoid casts that don't change the pointer type (#6785)
* Don't cast LOGICAL() to int* If the returned pointer type ever changes, the absence of the cast will give us a compiler warning due to the value being implicitly cast to an incompatible pointer type. An explicit cast, on the other hand, tells the compiler to silence the warning and do as written. * Drop more no-op casts * Introduce the Coccinelle linter For mistakes that are easy to fix automatically, Coccinelle patches may be written. If a patch finds a mistake, the linter will ask the human to fix either the code or the patch.
1 parent 7a531c8 commit f9c2824

File tree

10 files changed

+44
-15
lines changed

10 files changed

+44
-15
lines changed

.ci/linters/c/cocci_linter.R

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
cocci_linter = if (!nzchar(Sys.which("spatch"))) function(...) {} else function(c_obj) {
2+
bad <- FALSE
3+
for (spfile in list.files(".ci/linters/cocci", full.names = TRUE)) {
4+
# Coccinelle parser gets confused sometimes, so ignore stderr and the exit code
5+
out = suppressWarnings(system2(
6+
"spatch",
7+
shQuote(c(
8+
"--sp-file", spfile, c_obj$path, "--recursive-includes",
9+
"-I", R.home("include"), "-I", "src"
10+
)),
11+
stdout = TRUE, stderr = FALSE
12+
))
13+
if (length(out) > 0) {
14+
cat(sprintf("In file '%s', Coccinelle patch '%s' recommends the following changes:\n", c_obj$path, spfile))
15+
writeLines(out)
16+
bad <- TRUE
17+
}
18+
}
19+
if (bad) stop("Please apply the changes above or fix the linter")
20+
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
@@
2+
type T;
3+
T* E;
4+
@@
5+
- (T*)
6+
E

.github/workflows/code-quality.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ jobs:
3535
steps:
3636
- uses: actions/checkout@v4
3737
- uses: r-lib/actions/setup-r@v2
38+
- name: Install Coccinelle
39+
# relying on the action above us to have updated the package cache
40+
run: /usr/bin/sudo apt-get -y install coccinelle
3841
- name: Lint
3942
run: |
4043
linter_env = new.env()

src/assign.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1207,7 +1207,7 @@ void writeNA(SEXP v, const int from, const int n, const bool listNA)
12071207
memset(RAW(v)+from, 0, n*sizeof(Rbyte));
12081208
break;
12091209
case LGLSXP: {
1210-
int *vd = (int *)LOGICAL(v);
1210+
int *vd = LOGICAL(v);
12111211
for (int i=from; i<=to; ++i) vd[i] = NA_LOGICAL;
12121212
} break;
12131213
case INTSXP: {

src/coalesce.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ SEXP coalesce(SEXP x, SEXP inplaceArg) {
100100
if (val!=NA_INTEGER64) xP[i]=val; else if (final) xP[i]=finalVal;
101101
}
102102
} else {
103-
double *xP = (double *)REAL(first), finalVal=NA_REAL;
103+
double *xP = REAL(first), finalVal=NA_REAL;
104104
int k=0;
105105
for (int j=0; j<nval; ++j) {
106106
SEXP item = VECTOR_ELT(x, j+off);

src/fread.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ static const char* filesize_to_str(size_t fsize)
441441
double copyFile(size_t fileSize) // only called in very very rare cases
442442
{
443443
double tt = wallclock();
444-
mmp_copy = (char *)malloc((size_t)fileSize + 1 /* extra \0 */);
444+
mmp_copy = (char *)malloc(fileSize + 1 /* extra \0 */);
445445
if (!mmp_copy)
446446
return -1.0; // # nocov
447447
memcpy(mmp_copy, mmp, fileSize);

src/freadR.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,7 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
624624
resj++;
625625
if (type[j]!=CT_STRING && type[j]>0) {
626626
if (thisSize == 8) {
627-
double *dest = (double *)REAL(VECTOR_ELT(DT, resj)) + DTi;
627+
double *dest = REAL(VECTOR_ELT(DT, resj)) + DTi;
628628
const char *src8 = (char*)buff8 + off8;
629629
for (int i=0; i<nRows; ++i) {
630630
*dest = *(double *)src8;
@@ -633,7 +633,7 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
633633
}
634634
} else
635635
if (thisSize == 4) {
636-
int *dest = (int *)INTEGER(VECTOR_ELT(DT, resj)) + DTi;
636+
int *dest = INTEGER(VECTOR_ELT(DT, resj)) + DTi;
637637
const char *src4 = (char*)buff4 + off4;
638638
// debug line for #3369 ... if (DTi>2638000) printf("freadR.c:460: thisSize==4, resj=%d, %"PRIu64", %d, %d, j=%d, done=%d\n", resj, (uint64_t)DTi, off4, rowSize4, j, done);
639639
for (int i=0; i<nRows; ++i) {

src/ijoin.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ SEXP lookup(SEXP ux, SEXP xlen, SEXP indices, SEXP gaps, SEXP overlaps, SEXP mul
1010

1111
SEXP vv, tt, lookup, type_lookup;
1212
R_len_t *idx,*count,*type_count,xrows=INTEGER(xlen)[0],uxrows=LENGTH(VECTOR_ELT(ux, 0)),uxcols=LENGTH(ux);
13-
int *from = (int *)INTEGER(VECTOR_ELT(indices, 0));
14-
int *to = (int *)INTEGER(VECTOR_ELT(indices, 1));
13+
int *from = INTEGER(VECTOR_ELT(indices, 0));
14+
int *to = INTEGER(VECTOR_ELT(indices, 1));
1515
clock_t pass1, pass2, pass3, start;
1616
enum {ALL, FIRST, LAST} mult = ALL;
1717
enum {ANY, WITHIN, START, END, EQUAL} type = ANY;
@@ -31,8 +31,8 @@ SEXP lookup(SEXP ux, SEXP xlen, SEXP indices, SEXP gaps, SEXP overlaps, SEXP mul
3131
// For reference: uxcols-1 = type_count, uxcols-2 = count, uxcols-3 = type_lookup, uxcols-4 = lookup
3232
// first pass: calculate lengths first
3333
start = clock();
34-
count = (int *)INTEGER(VECTOR_ELT(ux, uxcols-2));
35-
type_count = (int *)INTEGER(VECTOR_ELT(ux, uxcols-1));
34+
count = INTEGER(VECTOR_ELT(ux, uxcols-2));
35+
type_count = INTEGER(VECTOR_ELT(ux, uxcols-1));
3636
switch (mult) {
3737
case FIRST:
3838
switch(type) {
@@ -225,10 +225,10 @@ SEXP overlaps(SEXP ux, SEXP imatches, SEXP multArg, SEXP typeArg, SEXP nomatchAr
225225

226226
R_len_t uxcols=LENGTH(ux),rows=length(VECTOR_ELT(imatches,0));
227227
int nomatch = INTEGER(nomatchArg)[0], totlen=0, thislen;
228-
int *from = (int *)INTEGER(VECTOR_ELT(imatches, 0));
229-
int *to = (int *)INTEGER(VECTOR_ELT(imatches, 1));
230-
int *count = (int *)INTEGER(VECTOR_ELT(ux, uxcols-2));
231-
int *type_count = (int *)INTEGER(VECTOR_ELT(ux, uxcols-1));
228+
int *from = INTEGER(VECTOR_ELT(imatches, 0));
229+
int *to = INTEGER(VECTOR_ELT(imatches, 1));
230+
int *count = INTEGER(VECTOR_ELT(ux, uxcols-2));
231+
int *type_count = INTEGER(VECTOR_ELT(ux, uxcols-1));
232232
SEXP lookup = VECTOR_ELT(ux, uxcols-4);
233233
SEXP type_lookup = VECTOR_ELT(ux, uxcols-3);
234234
SEXP ans, f1__, f2__, tmp1, tmp2;

src/negate.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ void negateByRef(SEXP x) {
55
error("not logical or integer vector"); // # nocov
66
}
77
const int n = length(x);
8-
int *ansd = (int *)LOGICAL(x);
8+
int *ansd = LOGICAL(x);
99
for(int i=0; i<n; ++i) {
1010
ansd[i] ^= (ansd[i] != NA_LOGICAL); // invert true/false but leave NA alone
1111
}

src/reorder.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ SEXP reorder(SEXP x, SEXP order)
6363
// This check is once up front, and then idx is applied to all the columns which is where the most time is spent.
6464
}
6565

66-
char *TMP = (char *)R_alloc(nmid, maxSize);
66+
char *TMP = R_alloc(nmid, maxSize);
6767

6868
for (int i=0; i<ncol; ++i) {
6969
const SEXP v = isNewList(x) ? VECTOR_ELT(x,i) : x;

0 commit comments

Comments
 (0)