Could you add a quickChangeLog entry? Emacs makes that simple withC-x 4 a from the file you changed.

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

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

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.

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?

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

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

Builds fails while generating the documentation

pdflatex is not available

I have not root privileges to install this missing package.

SeeR CMD build --help and ditto forR CMD check --help. You can skip the steps involving LaTeX.

$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 ... ERRORPackage 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

Why are you on R 3.0.2?

You can try an older version ofhighlight 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.

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’ ... WARNINGPackage vignettes without corresponding PDF/HTML:   ‘Rcpp-introduction.Rnw’   ‘Rcpp-unitTests.Rnw’* checking running R code from vignettes ... SKIPPED* checking re-building of vignette outputs ... SKIPPEDWARNING: There was 1 warning.NOTE: There was 1 note.See  ‘/home/fplazaonate/Rcpp.Rcheck/00check.log’for details.

Everything seems ok.

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.

Please try to addcache: false in you.travis.yml. It might work.@FPlaza

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 havesudo: required in there. Also, I see

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

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?

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.

Finally!
Champagne!

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

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_toffset(constsize_t& i,constsize_t& j)const {if( !::Rf_isMatrix(Storage::get__()) )thrownot_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 )throwindex_out_of_bounds() ;return i + nrow*j ;    }/**     * one dimensional offset doing bounds checking to ensure     * it is valid*/size_toffset(constsize_t& i)const {if(static_cast<R_xlen_t>(i) >= ::Rf_xlength(Storage::get__()) )throwindex_out_of_bounds() ;return i ;    }

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

if  (dim[0] <0 || dim[1] <0)throwindex_out_of_bounds();

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

Edit Yes, or your patch.

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.

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

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

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.

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

Tell me if it is ok now.

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

@kevinushey@jjallaire Any seconds?

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

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

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 Usheynotifications@github.com wrote:

Looks okay to be, but does changing the signature of the offset() function
constitute an ABI change? (@eddelbuettelhttps://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).

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

The ABI change tester was mostly as exasperated attempt to see if we could dosomething. 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'.

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.