Skip to content

Conversation

jmarshallnz
Copy link

Timings from R:

f1 <- cppFunction("LogicalVector is_empty(CharacterVector v) {
                     const int n = v.size();
                     LogicalVector e(n);
                     for (int i = 0; i < n; i++)
                       e[i] = v[i].size() == 0;
                     return e;
                  }")

f2 <- cppFunction("LogicalVector is_empty(CharacterVector v) {
                     const int n = v.size();
                     LogicalVector e(n);
                     for (int i = 0; i < n; i++)
                       e[i] = v[i].empty();
                     return e;
                  }")

chars <- rep(paste(rep("x", 100), collapse=""), 10000000)
chars[sample(10000000, 100000)] <- ""

> system.time(f1(chars))
   user  system elapsed 
  0.176   0.000   0.177 
> system.time(f2(chars))
   user  system elapsed 
  0.079   0.001   0.080 

Times improve rapidly as the string length gets longer, saving going via a string.

@eddelbuettel
Copy link
Member

As you can tell from the 'Failed' above, it does not pass the regression tests. As such we are unlikely to adopt it. Could you please try and and see why it fails?

@thirdwing
Copy link
Member

It seems a problem in the ppa (edd/misc). The test never started.

https://travis-ci.org/RcppCore/Rcpp/builds/56843011#L712

@eddelbuettel
Copy link
Member

Oh, sorry, am on vacation and with less-than-usual computer access. Failed tests / timed-out tests can just be restarted, which I just did. Sadly they now idle for a while before starting.

I may have to invoke apt-get with options directly rather than relying on travis-tool.sh if something changed, Either way I am on it now -- thanks for the heads-up.

@eddelbuettel
Copy link
Member

Looks like it worked. I will have to reconsider the pull request :)

@jmarshallnz
Copy link
Author

No worries - just a nice to have in some cases prior to taking the hit of going to std::string or similar for further processing.

@eddelbuettel
Copy link
Member

Learned something new in this issue ticket for Travis that I was cc'ed on: one now needs to declare a dependency on 'sudo' on new repos. The PR was a 'new repo' at least for @jmarshallnz and so this failed, but didn't when I re-ran it because my repo was pre-existing. Oh the joy :) At least now we have an explanation.

eddelbuettel added a commit that referenced this pull request Apr 19, 2015
adds empty() to [const_]string_proxy for more efficient checks for empty strings
@eddelbuettel eddelbuettel merged commit 869a5c3 into RcppCore:master Apr 19, 2015
eddelbuettel added a commit that referenced this pull request Apr 19, 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.

3 participants