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

Added explicit (const_)string_proxy/SEXP comparisons to resolve ambiguity #372

Merged
merged 1 commit into from
Sep 9, 2015
Merged

Conversation

dcdillon
Copy link
Contributor

@dcdillon dcdillon commented Sep 9, 2015

This fixes compile failures in readr, readxl, and rpg

@eddelbuettel
Copy link
Member

... and tidyr. Thanks for fixing it and submitting a PR while my rev.dep. check is still running!

@eddelbuettel
Copy link
Member

Ok, I checked and this fixed readr, readxl, rpg and tidyr. We still have package icd9 failing with:

** libs
ccache gcc -I/usr/share/R/include -DNDEBUG   -I"/usr/local/lib/R/site-library/Rcpp/include"  -fopenmp -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -g  -O3 -Wall -pipe
 -pedantic -std=gnu99 -c cutil.c -o cutil.o
ccache g++ -I/usr/share/R/include -DNDEBUG   -I"/usr/local/lib/R/site-library/Rcpp/include"  -I. -fopenmp -std=c++11 -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -g  
-O3 -Wall -pipe -Wno-unused -pedantic -c is.cpp -o is.o
ccache g++ -I/usr/share/R/include -DNDEBUG   -I"/usr/local/lib/R/site-library/Rcpp/include"  -I. -fopenmp -std=c++11 -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -g  
-O3 -Wall -pipe -Wno-unused -pedantic -c manip.cpp -o manip.o
manip.cpp: In function ‘Rcpp::String icd9AddLeadingZeroesMajorSingle(Rcpp::String)’:
manip.cpp:25:12: error: ambiguous overload for ‘operator==’ (operand types are ‘Rcpp::String’ and ‘SEXP’)
  if (major == NA_STRING) {
            ^
manip.cpp:25:12: note: candidates are:
In file included from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/Vector.h:67:0,
                 from /usr/local/lib/R/site-library/Rcpp/include/Rcpp.h:38,
                 from ./convert.h:22,
                 from manip.cpp:20:
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/String.h:414:14: note: bool Rcpp::String::operator==(const Rcpp::String&) const
         bool operator==( const Rcpp::String& other) const {
              ^
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/String.h:429:14: note: bool Rcpp::String::operator==(const const_StringProxy&) const
         bool operator==( const const_StringProxy& other) const {
              ^
/usr/lib/R/etc/Makeconf:143: recipe for target 'manip.o' failed
make: *** [manip.o] Error 1
make: *** Waiting for unfinished jobs....

@thirdwing
Copy link
Member

I think add those lines below into String.h should fix icd9.

bool operator==( SEXP other) const {                                
    return get_sexp() == other;                                     
}                                                                   

bool operator!=( SEXP other) const {                                
    return get_sexp() != other;                                     
 }

@eddelbuettel
Copy link
Member

Nice work, @thirdwing !

When I looked as vector/string_proxy.h I got into trouble with the forward reference. Going to String.h works as you say (and we do indeed need get_sexp() instead of get().

@dcdillon Do you want to add this to the PR or shall I just add it after merging?

edd@max:/tmp/rcpp-tmp/Rcpp$ diff -u ~/git/rcpp/inst/include/Rcpp/String.h inst/include/Rcpp/String.h                                                                                                                 
--- /home/edd/git/rcpp/inst/include/Rcpp/String.h       2015-09-08 09:59:23.570918337 -0500
+++ inst/include/Rcpp/String.h  2015-09-09 05:54:04.478869310 -0500
@@ -438,6 +438,14 @@
             return strcmp( get_cstring(), other.get_cstring() ) > 0;
         }

+        bool operator==( SEXP other ) const {
+            return get_sexp() == other;
+        }
+
+        bool operator!=( SEXP other ) const {
+            return get_sexp() != other;
+        }
+
     private:

         /** the CHARSXP this String encapsulates */
edd@max:/tmp/rcpp-tmp/Rcpp$ 

Ah shucks, I'll just do it and then start a new test run ...

eddelbuettel added a commit that referenced this pull request Sep 9, 2015
Added explicit (const_)string_proxy/SEXP comparisons to resolve ambiguity
@eddelbuettel eddelbuettel merged commit 0d483c7 into RcppCore:master Sep 9, 2015
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

Successfully merging this pull request may close these issues.

None yet

3 participants