ropensci / readODS

Read ODS (OpenDocument Spreadsheet) into R as data frame. Also support writing data frame into ODS file.
https://docs.ropensci.org/readODS/
Other
55 stars 22 forks source link

rchk issues #190

Closed chainsawriot closed 5 months ago

chainsawriot commented 5 months ago

https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/results/readODS.out

Cannot reproduce it locally.

docker pull kalibera/rchk:latest
docker run kalibera/rchk:latest readODS
chainsawriot commented 5 months ago

The Docker version doesn't match.

Rchk version: 1cae90e208e97a5c41f1c3e128d99b197478443e
R version: 84255/R Under development (unstable) (2023-04-13 r84255)
LLVM version: 14.0.0
chainsawriot commented 5 months ago

Confirmed with rhub

https://github.com/r-hub2/educated-yogic-starfish-readODS/actions/runs/9339195086/job/25703281095

chainsawriot commented 5 months ago

https://github.com/ropensci/readODS/blob/47df07c49bda9ac50301510fda93c63c23560098/inst/include/cpp11/r_vector.hpp#L912-L929

Changing to this

template <typename T>
inline r_vector<T>::operator SEXP() const {
  auto* p = const_cast<r_vector<T>*>(this);
  if (data_ == R_NilValue) {
    p->resize(0);
    return data_;
  }
  if (length_ < capacity_) {
    p->data_ = truncate(p->data_, length_, capacity_);
    SEXP nms;
    PROTECT(nms = names());
    auto nms_size = Rf_xlength(nms);
    if ((nms_size > 0) && (length_ < nms_size)) {
      nms = truncate(nms, length_, capacity_);
      names() = nms;
    }
    UNPROTECT(1);
  }
  return data_;
}

does not raise the rchk issues. But I also need to check valgrind.

chainsawriot commented 5 months ago

can't run rchk on duckdb. Don't know whether I should inform the developers of duckdb, which also uses the same method.

chainsawriot commented 5 months ago

valgrind is triple zero.

chainsawriot commented 5 months ago

Now I can directly attribute the rchk issues to the fix.

https://github.com/r-hub2/educated-yogic-starfish-rrrrrr/commit/04a1cab85376792442b42d6dc5dfc69cdafa79cd

Before and after.

chainsawriot commented 5 months ago

And this cancels the rchk issues.