Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed/unsigned comparison in IndexHash #1152

Closed
Bijaelo opened this issue Apr 9, 2021 · 9 comments
Closed

Signed/unsigned comparison in IndexHash #1152

Bijaelo opened this issue Apr 9, 2021 · 9 comments

Comments

@Bijaelo
Copy link

Bijaelo commented Apr 9, 2021

This is a reboot of an old issue from 2016 #574, that seems to be present once again while compiling under windows. The compiler throws a warning after compiling the below code. Assume the below minimum reproducible example is located in the working directory in a file called test.cpp. Calling sourceCpp("test.cpp") results in a warning.

Snippet from the warning:

C:/Users/olive/Documents/R/win-library/4.0/Rcpp/include/Rcpp/hash/IndexHash.h:103:35: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare].

Session info included at the very bottom.

#include <Rcpp.h>
using namespace Rcpp;

inline bool allin(const SEXP& x, const SEXP& table){
  if(TYPEOF(x) != TYPEOF(table))
    stop("x and table must be of the same type.");
  switch(TYPEOF(x)){
    case INTSXP:
      return is_true(all(Rcpp::in(Vector<INTSXP>(x), Vector<INTSXP>(table))));
    case REALSXP:
      return is_true(all(Rcpp::in(Vector<REALSXP>(x), Vector<REALSXP>(table))));
    case STRSXP:
      return is_true(all(Rcpp::in(Vector<STRSXP>(x), Vector<STRSXP>(table))));
    default:
      stop("Invalid type in allin [Type = %s, valid = (%s, %s, %s)]",
           Rf_type2char(TYPEOF(x)),
           Rf_type2char(INTSXP),
           Rf_type2char(REALSXP),
           Rf_type2char(STRSXP));
  }
}

// [[Rcpp::export]]
void testStuff(const SEXP& x, const SEXP& table){
  Rcout << allin(x, table) << '\n';
}

/***R
# Example call: Unnessary for the issue
x <- 1:6
table <- 1:20
testStuff(x, table) 
*/
>sessionInfo()
R version 4.0.3 (2020-10-10)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19042)

Matrix products: default

locale:
[1] LC_COLLATE=English_Denmark.1252  LC_CTYPE=English_Denmark.1252    LC_MONETARY=English_Denmark.1252 LC_NUMERIC=C                     LC_TIME=English_Denmark.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] Rcpp_1.0.6

loaded via a namespace (and not attached):
[1] compiler_4.0.3 tools_4.0.3  
@eddelbuettel
Copy link
Member

( Not fully complete reproducible example as you did not include a trivial call to testStuff with data of your choosing. )

@Bijaelo
Copy link
Author

Bijaelo commented Apr 9, 2021

Oh but to reproduce the "warning" no call to the function itself should be necessary. I am getting it when I compile through sourceCpp or alternatively build the package containing the function. So if you want a "full" reproducible example the only lacking piece should be library(Rcpp). :-)

@eddelbuettel
Copy link
Member

eddelbuettel commented Apr 9, 2021

(I understand that. I already reproduced it. I still ask you to add an example call so that everybody has the same run-time experience.)

I assume you are also aware of how to add flags such as Wno-sign-compare to the different CXX*FLAGS in ~/.R/Makevars? We covered that a few times here.

@Bijaelo
Copy link
Author

Bijaelo commented Apr 9, 2021

Of course, thank you however. I mostly placed this report as it seemed to be a regression from the #574 (which was apparently fixed?). In practice it is only a nuisance.

@eddelbuettel
Copy link
Member

Which is why it is not worth a) filing an issue (it is merely a warning) but if one insists as you do it is double foolish to do 95% of the work of an MCVE only to fail to add a call. But the worst is then to argue with me here 😁 instead of just saying "ah, yes" and editing the post.

@eddelbuettel
Copy link
Member

eddelbuettel commented Apr 9, 2021

You can carry the one-line fix over from 6a9ecc1.

(That comment was left pending for a few hours.)

@eddelbuettel
Copy link
Member

What I meant with callable example was that you did already really well with the C++ snippet above, Just add this at the end

/*** R
x <- 1:6
table <- 1:20
testStuff(x, table) 
*/

which then gets autocalled when you sourceCpp() reducing your R code above to

library(Rcpp)
sourceCpp(...)

which is really Rcpp::sourceCpp(...) and implicit 😀 But the /*** R .... */ is pure gold. In case you didn't know.

@Bijaelo
Copy link
Author

Bijaelo commented Apr 10, 2021

True, I didn't think of inlining the code into the Cpp snippet. I don't really use that feature day to day. 😀

And thanks for the quick fix. I've cloned it, as I'm likely doomed to forget setting the flags once in a while (and I prefer to stay out of Makeconf). 😁
Ps. I am from Denmark, so any comment left hanging may potentially be caused by the time difference. ☺️

@eddelbuettel
Copy link
Member

and I prefer to stay out of Makeconf

Don't. It is useful, and documented in Writing R Extensions. We only have a small number of hooks. This is a potent one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants