From: Will N. <wi...@he...> - 2010-04-26 19:31:33
|
Hi all, I noticed that the code SWIG 1.3.40 generates for wrapping a std::string uses a defunct method for creating a string in R: SWIGINTERN SEXP SWIG_FromCharPtrAndSize(const char* carray, size_t size) { SEXP t, c; if (!carray) return R_NilValue; Rf_protect(t = Rf_allocVector(STRSXP, 1)); c = Rf_allocVector(CHARSXP, size); strncpy((char *)CHAR(c), carray, size); SET_STRING_ELT(t, 0, c); Rf_unprotect(1); return t; } Since R 2.6.0 (current version being >= 2.10.1), Rf_allocVector of a CHARSXP type generates the following error at run time: > foo$getString() Error in foo$getString() : use of allocVector(CHARSXP ...) is defunct (assuming that getString was a class method declared as: "std::string testClass::getString() const") Here are the release notes and the relevant section from the R docs: https://stat.ethz.ch/pipermail/r-announce/2007/000467.html o There is now a global CHARSXP cache, R_StringHash. CHARSXPs are no longer duplicated and must not be modified in place. Developers should strive to only use mkChar (and mkString) for creating new CHARSXPs and avoid use of allocString. A new macro, CallocCharBuf, can be used to obtain a temporary char buffer for manipulating character data. This patch was written by Seth Falcon. Ironically, returning a "char *" does the right thing, in that it calls Rf_mkString on the immediate result of the C++ code: result = (char *)((testClass const *)arg1)->getString2(); r_ans = result ? Rf_mkString(reinterpret_cast< char * >(result)) : R_NilValue; (assuming a method definition "char *testClass::getString2() const") This works fine, *but* it now has the undesirable problem that it seems not to be possible to use newfree: %typemap(newfree) char * "delete [] $1;" %newobject testClass::getString2(); The above has no effect for R, but the output language can be changed to Perl and whatever was defined in the newfree gets correctly inserted into the generated code. In the case of returning a client-allocated char *, this verifiably (valgrind) results in a memory leak for obvious reasons. My proposal is: 1) For std::string, change SWIG_FromCharPtrAndSize to use Rf_mkString instead of Rf_allocVector. a) Maybe it is also possible to intercept SWIG_From_std_string for the R case and have it just do this directly -- since R strings are C-strings, there is no point to supporting raw binary (the std::string::data() and ::size() methods). 2) For newfree, get it working so it is possible to use it with R. Item 1 is higher on my priority list. My question is: what branch should I make my changes against (1.3.40, 2.0.0, or...?) and any pointers to FAQs etc on procedures for making/testing/submitting changes would be greatly appreciated. Many thanks, Will |
From: William S F. <ws...@fu...> - 2010-04-29 06:42:52
|
Will Nolan wrote: > > My question is: what branch should I make my changes against (1.3.40, 2.0.0, or...?) and any pointers to FAQs etc on procedures for making/testing/submitting changes would be greatly appreciated. > Changes should be made against trunk which will form the basis of the next release (2.0.0). Can you make the change so that older versions of R still work? William |
From: Will N. <wi...@he...> - 2010-05-03 18:53:23
|
> Changes should be made against trunk which will form the basis of the > next release (2.0.0). Can you make the change so that older versions of > R still work? It looks like in Lib/r/rrun.swg there is code that checks the R version from the R header files: #if R_VERSION >= R_Version(2,6,0) #define VMAXTYPE void * #else #define VMAXTYPE char * #endif Is it possible to do the same check within std_string.i? If so, then the same version check could be used to include or not include both the new code in std_string.i, as well as remove or not remove the old code in rfragments.swg. The only issue I can think of is that I don't have an older copy of R that I can do any regression testing against. Given that it's just a case of including or not including code, it theoretically shouldn't break for < 2.6.0 R but it would be best to check. What do you think is the best course of action here? Thanks, Will |