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

Adds Known Issues section to Rcpp FAQ (closes #628, #563, #552, #460, #419, and #251) #661

Merged
merged 2 commits into from Mar 31, 2017

Conversation

Projects
None yet
4 participants
@coatless
Contributor

coatless commented Mar 28, 2017

Added a "Known Issues" section to the Rcpp FAQ vignette

Covers issues:

#628, #563, #552, #460, #419, and #251

Modified with @nathan-russell comments: Rcpp-FAQ.pdf

@eddelbuettel

I think we integrate this and then make one or two passes over it for smaller issues and cosmetics.

Thanks for putting this together. This Section 5 was overdue.

@nathan-russell

This comment has been minimized.

Show comment
Hide comment
@nathan-russell

nathan-russell Mar 29, 2017

Contributor

@coatless This looks great, nice work! Making a pass over section 5, I found only a few (minor) things that could potentially be fixed.


5.1, last paragraph

"With this being said, there is one last area of contention [...]"

I'm pretty sure all compilers should be able to enforce constness, regardless of the quirks associated with references & the proxy model. For example,

// [[Rcpp::export]]
NumericVector modify_const(const NumericVector x)
{
    // x[0] = x[0] + 1.0;
    //
    // error: assignment of read-only location
    // 'x.Rcpp::Vector<RTYPE, StoragePolicy>::operator[]<
    //      14, Rcpp::PreserveStorage>(0ll)'
    //  x[0] = x[0] + 1.0;
    //       ^
    //
    // x = NumericVector::create(1);
    //
    // error: passing 'const NumericVector
    // {aka const Rcpp::Vector<14, Rcpp::PreserveStorage>}'
    // as 'this' argument of
    // 'Rcpp::Vector<RTYPE, StoragePolicy>& Rcpp::Vector<
    //      RTYPE, StoragePolicy>::operator=
    //      (const Rcpp::Vector<RTYPE, StoragePolicy>&)
    //  [with int RTYPE = 14; StoragePolicy = Rcpp::PreserveStorage]'
    //  discards qualifiers [-fpermissive]
    //  x = NumericVector::create(1);
    //    ^
    return x;
}

5.3, first paragraph, first sentence

"Assignment using the operator= with either Vector and Matrix classes will not illicit an element-wise fill."

This should be elicit rather than illicit.


5.3, second paragraph, first sentence

"The implementation of the operator= for the Vector class will replace the existing vector with the assignee value."

Possible typo -- should be 'assigned'? 'assignee' would be the object that the new value is being assigned to (i.e. the Vector in this context).


5.3, second paragraph, first sentence

"This behavior is valid even if the assignee value is a [...]"

Likewise, I believe 'assignee' should be 'assigned' here.


As Dirk noted, these could just as well be addressed after merging the PR. All in all, this is a great addition to the FAQ vignette.

Contributor

nathan-russell commented Mar 29, 2017

@coatless This looks great, nice work! Making a pass over section 5, I found only a few (minor) things that could potentially be fixed.


5.1, last paragraph

"With this being said, there is one last area of contention [...]"

I'm pretty sure all compilers should be able to enforce constness, regardless of the quirks associated with references & the proxy model. For example,

// [[Rcpp::export]]
NumericVector modify_const(const NumericVector x)
{
    // x[0] = x[0] + 1.0;
    //
    // error: assignment of read-only location
    // 'x.Rcpp::Vector<RTYPE, StoragePolicy>::operator[]<
    //      14, Rcpp::PreserveStorage>(0ll)'
    //  x[0] = x[0] + 1.0;
    //       ^
    //
    // x = NumericVector::create(1);
    //
    // error: passing 'const NumericVector
    // {aka const Rcpp::Vector<14, Rcpp::PreserveStorage>}'
    // as 'this' argument of
    // 'Rcpp::Vector<RTYPE, StoragePolicy>& Rcpp::Vector<
    //      RTYPE, StoragePolicy>::operator=
    //      (const Rcpp::Vector<RTYPE, StoragePolicy>&)
    //  [with int RTYPE = 14; StoragePolicy = Rcpp::PreserveStorage]'
    //  discards qualifiers [-fpermissive]
    //  x = NumericVector::create(1);
    //    ^
    return x;
}

5.3, first paragraph, first sentence

"Assignment using the operator= with either Vector and Matrix classes will not illicit an element-wise fill."

This should be elicit rather than illicit.


5.3, second paragraph, first sentence

"The implementation of the operator= for the Vector class will replace the existing vector with the assignee value."

Possible typo -- should be 'assigned'? 'assignee' would be the object that the new value is being assigned to (i.e. the Vector in this context).


5.3, second paragraph, first sentence

"This behavior is valid even if the assignee value is a [...]"

Likewise, I believe 'assignee' should be 'assigned' here.


As Dirk noted, these could just as well be addressed after merging the PR. All in all, this is a great addition to the FAQ vignette.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 30, 2017

Codecov Report

Merging #661 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #661   +/-   ##
=======================================
  Coverage   92.91%   92.91%           
=======================================
  Files          65       65           
  Lines        3303     3303           
=======================================
  Hits         3069     3069           
  Misses        234      234

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 876d635...d149f51. Read the comment docs.

Codecov Report

Merging #661 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #661   +/-   ##
=======================================
  Coverage   92.91%   92.91%           
=======================================
  Files          65       65           
  Lines        3303     3303           
=======================================
  Hits         3069     3069           
  Misses        234      234

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 876d635...d149f51. Read the comment docs.

@eddelbuettel eddelbuettel merged commit 886f5df into RcppCore:master Mar 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment