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

Reduce -Wconversion warnings for sugar code #688

Merged
merged 1 commit into from May 7, 2017

Conversation

@krlmlr
Copy link
Contributor

commented May 5, 2017

2/n.

@@ -133,7 +133,8 @@ inline Vector<STRSXP> trimws(const Vector<STRSXP>& x, const char* which = "both"
}

inline Matrix<STRSXP> trimws(const Matrix<STRSXP>& x, const char* which = "both") {
R_xlen_t i = 0, nr = x.nrow(), nc = x.ncol(), sz = x.size();
R_xlen_t i = 0, sz = x.size();
int nr = x.nrow(), nc = x.ncol();

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel May 5, 2017

Member

Just double-checking here: Even with R_xlen_t, rows and cols are still int in R itself, but number of matrix elements can exceed int. Do I have that correct?

This comment has been minimized.

Copy link
@krlmlr

krlmlr May 5, 2017

Author Contributor

I think that's the way it's implemented currently, see R-ints.

This comment has been minimized.

Copy link
@coatless

coatless May 5, 2017

Contributor

Per https://stat.ethz.ch/R-manual/R-devel/library/base/html/LongVectors.html

Arrays (including matrices) can be based on long vectors provided each of their dimensions is at most 2^31 - 1:

Type Storage size Value range
int 2 or 4 bytes -32,768 to 32,767 or -2,147,483,648 to 2,147,483,647

In theory, you are bounded by: (2^31 - 1)^2 = 4,611,686,014,132,420,609

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel May 5, 2017

Member

Me too, was just trying to confirm. Because otherwise nrow, ncol should not be int here and in Matrix.

Have a test running with the now-merged PR. Things take some time of course.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel May 5, 2017

Member

Not uint though. R only uses signed ints.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel May 5, 2017

Member
R> .Machine$integer.max
[1] 2147483647
R> 

This comment has been minimized.

Copy link
@coatless

coatless May 5, 2017

Contributor

Quoted the 2byte bounds instead of 4byte blah.

@codecov-io

This comment has been minimized.

Copy link

commented May 5, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #688   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files          66       66           
  Lines        3511     3511           
=======================================
  Hits         3152     3152           
  Misses        359      359

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 5f20d1a...d4ecba1. Read the comment docs.

Copy link
Member

left a comment

Looks good to me. Let's get a second nod from someone and I'll put it in.

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Tests are good so far (just committed for #687) so in it goes with a fresh test.

@eddelbuettel eddelbuettel merged commit 4e50e2b into RcppCore:master May 7, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@krlmlr krlmlr deleted the krlmlr:f-Wconversion-2 branch Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.