Add at accessors with bounds checking

[fplazaonate]

I just made a copy of the operators() by making sure that bound are checked.

[eddelbuettel]

Could you add a quick ChangeLog entry? Emacs makes that simple with C-x 4 a from the file you changed.

A new unit tests or two would be awesome too...

[fplazaonate]

I am quite lost here.
Could you tell me what is wrong?

[eddelbuettel]

That looks like a Travis issiue:

Found more than one class "Rcpp_World" in cache; using the first, from namespace '.GlobalEnv'

is not something I have ever seen.

[eddelbuettel]

RcppArmadillo tested just fine a second ago.

For argument's sake, can you revert to before the errors started? It looks like build 891 for PR #344 passed but then starting with build 894 for PR #342 you hit trouble. Can you go back before that?

[eddelbuettel]

Maybe we should reopen this? We still need to understand why Travis fails here. Is this a feature of anything here?

[eddelbuettel]

You are clearly having trouble with Travis CI. So let's look at this from a different angle. What happens when you R CMD build rcpp and R CMD check Rcpp_0.12.0.1.tar.gz on your machine?

[fplazaonate]

Builds fails while generating the documentation

pdflatex is not available

I have not root privileges to install this missing package.

[eddelbuettel]

See R CMD build --help and ditto for R CMD check --help. You can skip the steps involving LaTeX.

[fplazaonate]

$R CMD build --no-manual --no-build-vignettes Rcpp/
* checking for file ‘Rcpp/DESCRIPTION’ ... OK
* preparing ‘Rcpp’:
* checking DESCRIPTION meta-information ... OK
* cleaning src
* running ‘cleanup’
* installing the package to process help pages
* cleaning src
* running ‘cleanup’
* checking for LF line-endings in source and make files
* checking for empty or unneeded directories
* building ‘Rcpp_0.12.0.1.tar.gz’

_R_CHECK_FORCE_SUGGESTS_=0 R CMD check --no-manual --no-vignettes --no-build-vignettes Rcpp_0.12.0.1.tar.gz
* using log directory ‘/home/fplazaonate/Rcpp.Rcheck’
* using R version 3.1.1 (2014-07-10)
* using platform: x86_64-unknown-linux-gnu (64-bit)
* using session charset: UTF-8
* using options ‘--no-vignettes --no-build-vignettes’
* checking for file ‘Rcpp/DESCRIPTION’ ... OK
* this is package ‘Rcpp’ version ‘0.12.0.1’
* checking package namespace information ... OK
* checking package dependencies ... ERROR
Package suggested but not available for checking: ‘highlight’

VignetteBuilder package required for checking but installed: ‘highlight’

I can't install the package 'highlight' because it's not available for R version 3.0.2

[eddelbuettel]

Why are you on R 3.0.2?

You can try an older version of highlight from its archive section. You can also locally edit away the Suggests: for Rcpp as highlight is needed only for the vignettes you are skipping anyway.

[fplazaonate]

I work on R 3.0.2 because this is the version installed on our server.
Here is the output after the modifications you suggested:

R CMD check --no-manual --no-vignettes --no-build-vignettes Rcpp_0.12.0.1.tar.gz
...
* checking tests ...
  Running ‘doRUnit.R’
 OK
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes in ‘inst/doc’ ... WARNING
Package vignettes without corresponding PDF/HTML:
   ‘Rcpp-introduction.Rnw’
   ‘Rcpp-unitTests.Rnw’

* checking running R code from vignettes ... SKIPPED
* checking re-building of vignette outputs ... SKIPPED

WARNING: There was 1 warning.
NOTE: There was 1 note.
See
  ‘/home/fplazaonate/Rcpp.Rcheck/00check.log’
for details.

Everything seems ok.

[eddelbuettel]

Ok, that looks better. One step in the right direction. Now we need to figure out why Travis CI no longer likes your commits. I don't have a good idea for that other than bisecting / backtracking to what worked before.

[thirdwing]

Please try to add cache: false in you .travis.yml. It might work. @fplaza

[eddelbuettel]

Nice. I was also thinking that it may be the old-vs-new Travis setup but couldn't quite think through why it was new for me and old for @fplaza. And we do have sudo: required in there. Also, I see

This job ran on our legacy infrastructure. Please read our docs on how to upgrade

[fplazaonate]

Disabling the cache doesn't work.
According to the (doc)[http://docs.travis-ci.com/user/caching], an administrator can clear the cache from the web interface.
Could you give it a try?

[fplazaonate]

Test fails because Rcpp uses wrong integer types so "v.at(-1)" is considered valid.
I am going to disable some tests temporarily but this issue should be fixed.

[fplazaonate]

Finally!
Champagne!

[eddelbuettel]

Hm. There is something about this that I do not like. If we think that bounds checking has merit and the type of the index is int then surely we should be able to cope with (erroneous) negative indices just as we deal with the positive-but-too-large case, no?

[fplazaonate]

No, because 'at' accessors rely on the following methods which are ill-typed.
Here are the changes needded: 5a8911e

   /**
     * offset based on the dimensions of this vector
     */
    size_t offset(const size_t& i, const size_t& j) const {
        if( !::Rf_isMatrix(Storage::get__()) ) throw not_a_matrix() ;

        /* we need to extract the dimensions */
        int *dim = dims() ;
        size_t nrow = static_cast<size_t>(dim[0]) ;
        size_t ncol = static_cast<size_t>(dim[1]) ;
        if( i >= nrow || j >= ncol ) throw index_out_of_bounds() ;
        return i + nrow*j ;
    }

    /**
     * one dimensional offset doing bounds checking to ensure
     * it is valid
     */
    size_t offset(const size_t& i) const {
        if( static_cast<R_xlen_t>(i) >= ::Rf_xlength(Storage::get__()) ) throw index_out_of_bounds() ;
        return i ;
    }

[eddelbuettel]

So how about adding this just after the assignment to *dim:

if  (dim[0] < 0 || dim[1] < 0) throw index_out_of_bounds();

with (optionally) a message about non-permissible negative indices?

Edit Yes, or your patch.

[fplazaonate]

This looks quite hackish.
The changes that I proposed to make Rcpp coherent with RInternals look more robust to me.
I will open a new issue to discuss this.

[eddelbuettel]

Slow down, we HAVE an open issue to discuss this. This one. What would a new one add?

[fplazaonate]

Because it is strongly related to a problem which is more general that we discussed before:
#338

[eddelbuettel]

Of course. And negative values is as much a violation as are beyond-size values. Your patch was missing that, now it has it. Better.

[fplazaonate]

I was for a patch that solves the problem globally but I can propose one which solves it just for the "offset" methods.

[fplazaonate]

Tell me if it is ok now.

[eddelbuettel]

Sorry, we're all a little swamped. It looks pretty good to me.

@kevinushey @jjallaire Any seconds?

[fplazaonate]

No worries. I was just wondering if you received a notification of my commit.

[kevinushey]

Looks okay to be, but does changing the signature of the offset() function constitute an ABI change? (@eddelbuettel, I vaguely recall you said you had a way of testing that?)

[jjallaire]

We can change interfaces and signatures without breakage so long as they
are implemented exclusively in header files. Is that the case here?

On Aug 21, 2015, at 5:21 PM, Kevin Ushey notifications@github.com wrote:

Looks okay to be, but does changing the signature of the offset() function
constitute an ABI change? (@eddelbuettel https://github.com/eddelbuettel,
I vaguely recall you said you had a way of testing that?)

—
Reply to this email directly or view it on GitHub
#342 (comment).

[kevinushey]

That is indeed the case here, so I think we're safe.

[eddelbuettel]

The ABI change tester was mostly as exasperated attempt to see if we could do something. As I recall, I use a prebuilt binary package (on Travis) and try to load it. I don't think this would detect a changed-but-not-used-here alteration to function signatures. It may be better than not testing at all but I think it is still far from perfect.

That said, I think this change does look as if it should be 'clean'.

[eddelbuettel]

Am I missing something or should the msg be "index out of bounds" because the fact that we have an exception means your throw went into effect?

[fplazaonate]

Actually, the error message is written if there is no error generated (c.f. RUnit documentation)

[eddelbuettel]

I guess you are correct -- if it passes we just get TRUE. But if you look at our code the msg field, when we set it all :), mostly just states where this came from. So "matrix bounds check" would be fine, and s/matrix/vector/ for the other one.

Not that important. I'll try merge this maybe late this evening if I get to it. It now needs a manual intervention because ChangeLog changed in the meantime ...

[eddelbuettel]

Ok, at long last this has been merged. Sorry again for the earlier trouble with the (entirely unrelated) issue caused by R 3.2.2 and unit testing, and thanks for the contribution, and your patience in seeing it through.