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

support for r 3.0.0 #87

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

javierluraschi
Copy link

Add support for r 3.0.0 to allow upstream packages like tibble to support 3.0.0 as well.

src/name.c Outdated
@@ -22,12 +22,33 @@ SEXP as_name(SEXP x) {
}
}

SEXP shallow_duplicate(SEXP list)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a comment to mark the source and license. And you'll need to add R-core to Authors@R in the DESCRIPTION

@@ -11,7 +11,7 @@ Authors@R: c(
License: GPL-3
LazyData: true
Depends:
R (>= 3.1.0)
R (>= 3.0.0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also modify the travis file to enforce this? (Ask @jimhester for details)

Copy link
Contributor

@jimhester jimhester Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earliest travis version available on travis is 3.0.3, however there was a devtools bug in the CRAN version that prevents installation of packages with 3.0.3 (https://travis-ci.org/hadley/devtools/jobs/172912550#L1115). You will need to install devel devtools manually by adding something like the following to .travis.yml (untested).

r:
 - 3.0.3
 - oldrel
 - release
 - devel

before_install:
  - if [[ "$TRAVIS_R_VERSION_STRING" = '3.0.3' ]]; then git clone https://github.com/hadley/devtools ~/devtools && pushd ~/devtools && git fetch origin pull/1388/head:3.0.3 && git checkout 3.0.3 && R CMD build . && R CMD INSTALL devtools*tar.gz && popd;fi

install:
  - R -e 'devtools::install_deps(dep = T)'

@javierluraschi
Copy link
Author

@hadley some tests are failing with the port from the original shallod_duplicate function. Instead, seems we should look at porting a more recent version: https://github.com/wch/r-source/blob/af7f52f70101960861e5d995d3a4bec010bc89e6/src/main/duplicate.c

Now... it looks like shallow_duplicate and duplicate are now sharing code through duplicate1. Being that the case, we could replace the call to shallow_duplicate with duplicate. However, I don't fully understand what would be the side effects of using duplicate in older versions of R that do not provide a shallow copy.

Would this be a performance downgrade? Or would this break lazyeval.

Happy to keep this PR open but it definitely needs review from more experienced folks familiar in the r core codebase and lazyeval.

@codecov-io
Copy link

codecov-io commented Nov 3, 2016

Current coverage is 97.27% (diff: 100%)

Merging #87 into master will decrease coverage by 1.02%

@@             master        #87   diff @@
==========================================
  Files            15         15          
  Lines           354        367    +13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            348        357     +9   
- Misses            6         10     +4   
  Partials          0          0          

Powered by Codecov. Last update c155c3d...c90aa66

@hadley
Copy link
Owner

hadley commented Nov 3, 2016

It's probably a minor performance regression, but it seems like it's going to be challenging to make lazyeval backward compatible with R 3.0.0.

@@ -27,7 +27,7 @@ SEXP lhs_name(SEXP x) {
Rf_errorcall(R_NilValue, "`x` must be a list (not a %s)", Rf_type2char(TYPEOF(x)));

int n = Rf_length(x);
SEXP x2 = PROTECT(Rf_shallow_duplicate(x));
SEXP x2 = PROTECT(Rf_duplicate(x));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Rf_shallow_duplicate() if it is available? In this case any potential regressions are limited to R 3.0 users. We should then also check if the R version lazyeval was built with a different R version in .onLoad().

krlmlr added a commit to tidyverse/tibble that referenced this pull request Jan 9, 2017
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.

5 participants