search

kalibera / rchk

102 stars 10 forks source link

Could "// norchk" be a recognized marker for false positives? #9

Closed mattdowle closed 6 years ago

mattdowle commented 6 years ago

i.e. just like the # nocov marker in the covr project here: https://github.com/r-lib/covr#exclusion-comments

I managed to reduce 189 rchk messages down to 3 for package data.table.

Package data.table version 1.11.2
Package built using 74708/R 3.6.0; x86_64-pc-linux-gnu; 2018-05-11 11:48:39 UTC; unix   
Checked with rchk version 63f79d910f5835174fcaa5a0a7d2409348f7d2ac
More information at https://github.com/kalibera/cran-checks/blob/master/rchk/PROTECT.md

Function allocateDT
  [PB] has possible protection stack imbalance data.table/src/freadR.c:378

Function preprocess
  [PB] has possible protection stack imbalance data.table/src/fmelt.c:352

Function userOverride
  [PB] has possible protection stack imbalance data.table/src/freadR.c:310

This was a very helpful exercise and I restructured wherever I could. The three remaining are all because of an allocVector() in one C function, which is passed to another C function which does the UNPROTECT() before returning to R level, as described in the excellent rchk documentation here. The fmelt.c one could possibly be restructured (with difficulty). But the two in freadR.c are because that file contains the R-API preamble which then calls fread.c which is R-API independent. fread.c is pure C code and is shared with the pydatatable project for Python. When fread.c is done populating the R vectors that freadR.c created, it returns to freadR.c which UNPROTECTs and then returns to R level. I can't see any way of restructuring that to pass rchk.

If I could add a comment on those lines, something like "// norchk", then I could mark those false positives as investigated and checked. This would then resolve and remove the rchk additional issues on the CRAN check page so that both CRAN maintainers and users can see that all issues have been dealt with.

If this isn't already possible, I'm not asking you to implement it, necessarily. I'm just asking if this approach is acceptable in principle. Then I can perhaps submit a PR to achieve this.

Best, Matt

kalibera commented 6 years ago

An easy way to restructure the code around allocateDT would be to rely on implicit protection. You would have a container (a cons cell) already protected before calling freadMain from freadR. allocateDT would just store the pointer to the newly allocated object into that container, by which it would be implicitly protected. Unrelated to protect bug detection, but I think DT would better be passed around in function arguments all the way to allocateDT, relying on a global (yet file-static) variable makes the code hard to read. The R-independent middle layer could use void * to type such argument.

mattdowle commented 6 years ago

Ok that should work, thanks. No need for this issue then (closing) and reopening on the data.table side.