-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Cannot sapply lambda functions #213
Comments
Oooof. Would be nice to have that working. Any idea of what to change / how to change it? Any chance you could cook up a PR? |
You need Good luck with conditional compilation. I probably don't want to deal with that. |
We have conditioned on 'do we have more than C++98' for many years within Rcpp, so there is no technical reason not to. Many other projects do as well as eg Armadillo. But yes, it is work, and as always with all of us being volunteers here this may or may not get done. |
I am well aware of these technicalities. I was just saying that I don't fancy being a part of it. |
Sure. But at the same time you also cut off a lot of users. We just saw that with RcppArmadillo where the last version tickled a bug in g++-4.4 which, however you and I may feel about a version that old, is still used in a lot of places. |
Just a heads up, in case this may be of some use: It appears that patching For testing purposes, I've experimented with replacing the code defining #include <type_traits>
namespace Rcpp { namespace traits { using std::result_of; } } This appears to work for However, there are still compilation errors for the Romain's lambda example:
Not quite sure what's the cause here -- could it be that In any case, if making
On a side note: It's header-only and already a part of the |
Thanks for the follow-up. I have at times considers whether we'd want to depend on BH and it is tempting: header-only, (mostly) stable, well-tested, widely used, ... But it would add a pretty big package to all builds so it adds quite some weight. Pulling small things out of Boost is also an option, we already have a macro processor borrowed from it. Do you feel like working some more on this? |
In my experience it's hard to pull only "a little" out of boost. I think On Sat, May 2, 2015 at 11:43 AM, Dirk Eddelbuettel <notifications@github.com
|
But build time is every Travis run for those working from source (rather than, say, my PPA). So BH would be expensive to some. At least until the caching gets better... |
So they'd have to pay the BH download on every travis run. Not On Sat, May 2, 2015 at 12:16 PM, Dirk Eddelbuettel <notifications@github.com
|
I'd word it exactly the same way. On the margin, leaner Rcpp is better but we may get to a point where depending on BH will be a good idea. So let's keep the doors open... |
@eddelbuettel: can take a look, provided someone walks me through / gives hints on the encountered issue with @jjallaire definitely makes a good point. While Boost has been improving its modularization (since around 2013; cf. https://svn.boost.org/trac/boost/wiki/CMakeModularizationStatus & https://isocpp.org/blog/2013/11/boost-migrating) -- resulting in independent, library-specific repositories (here: https://github.com/boostorg/utility), at the same time I can see that Some dependencies on Boost.MPL seem trivial / easily removable (AFAICT, it all boils down to the equivalents of C++11's That being said, I think it's an "I cross that bridge when I come to it" matter at the moment for the issue at hand -- if cooperation with |
My opinions on this are somewhat mixed. I also prefer Rcpp being lean, but am open to bringing in Boost / BH if it has a large enough benefit. I am not sure if the benefit is large enough in this case. One thing to worry about is, if Rcpp decides to depend on boost, it could break compilation on older compilers (e.g. if the release of Boost packaged in BH is somehow incompatible with an older GCC). One of the main drives for Rcpp to stick with C++98 is to ensure it can compile with such older compilers. (I could be wrong here -- I am not sure how committed Boost is in maintaining backwards compatibility with older compilers...) At the same time, I feel that if a user really wants C++11 features, they should just use Rcpp11 or (if @romainfrancois plans to begin work on it soon) Rcpp14. Finally, in my opinion, allowing lambdas to work on Rcpp types isn't a strong enough reason to bring BH. You can still use lambdas just fine on STL vectors, or even on an iterator pair constructed from an Rcpp vector, e.g.
Of course, the semantics aren't quite the same as |
Depending on BH means each and every package that uses Rcpp will have to have LinkingTo: Rcpp, BH Not a huge deal but still. Rcpp already has some conditional compilation on c++11, that can easily be used to work around this. And lambdas are c+11 anyway. So if you do want to support this C++11 feature, the best is to actually use C++11. That should be easy enough, and you can easily borrow from Rpp11 or Rcpp14. |
I didn't consider the fact that depending on BH would retroactively require all of our dependent packages to do |
I think we are all on the same page: we could deploy BH if there was sufficient need/merit; nobody had yet claimed that this issue called for it. All good... |
OK, I have a patch proposal! :-) One thing to note is that
Note how the This is no big loss, however, since we can work around it -- the usage in the Thus, the desired result-type type is the result of the function First idea (solely to check whether this form of replacement can work): Making use of the following:
We arrive at (initially C++11-only, generalized below), placed in "sapply.h" (reasoning follows):
The rationale behind placing this in The rationale behind the Now, we systematically replace every occurrence of Now, in order to move to conditional compilation, I think we need something like the following
See: http://en.cppreference.com/w/cpp/preprocessor/replace Note how this mirrors There is a somewhat different macro Finally: We can now define:
Tested with freestanding functions (including passed-by-pointer and passed-by-reference), function objects, and lambdas. Now, for C++11-and-newer compilers this brings support for lambdas -- while, at the same time, gracefully reducing to the current code for the pre-C++11 compilers :-) |
One note to keep in mind. You referenced the Rtools compiler (4.6.3) as On Mon, May 11, 2015 at 3:20 PM, Matt notifications@github.com wrote:
|
@jjallaire: Sure! The aforementioned workarounds are very much motivated by this aspect ;-) |
Minor comment re your comment about our somewhat inconsistent "is this C++11" macros: these grew over the years and sometimes out of simple needs (eg |
@eddelbuettel: Exactly! Ideally (read: if the supported compilers were more recent), it would probably be best to use the "Feature-testing recommendations for C++": Or, in any case, to still use feature-level testing granularity (testing for the level of "supported standard as a whole" can generally get brittle and unreliable): For instance, something along the following lines (or even However, the "more recent" in this context means either GCC 5 or Clang 3.4:
In other words, probably not applicable to Rcpp any time soon... Incidentally (back to supporting the not-so-recent compilers), this is also the reason for the elaborate dance done by On a side note (and completely independently from the issue at hand): I'm wondering, given that these macros are a part of Boost.Config, how self-contained is the Perhaps rejuvenating the codebase to make use of the feature macros present there would be the closest one could get to the feature-testing recommendations in practice (including doing things right w.r.t. the granularity). This is a significant task on its own, naturally. |
Resolve #213: Cannot sapply lambda functions
With the cpp11 plugin on, Rcpp cannot sapply lambda functions.
I get :
This is mainly
Rcpp::traits:: result_of
's fault.The text was updated successfully, but these errors were encountered: