Skip to content

Remove cast to R_xlen_t (closes #1334) #1335

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Remove cast to R_xlen_t (closes #1334) #1335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
2024-09-29 Dirk Eddelbuettel <edd@debian.org>

* inst/include/Rcpp/vector/Vector.h: Remove a cast as R_xlen_t
is returned now

2024-09-17 Dirk Eddelbuettel <edd@debian.org>

* DESCRIPTION (Version, Date): Roll micro version
Expand Down
4 changes: 2 additions & 2 deletions inst/include/Rcpp/vector/Vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Vector.h: Rcpp R/C++ interface class library -- vectors
//
// Copyright (C) 2010 - 2023 Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2010 - 2024 Dirk Eddelbuettel and Romain Francois
//
// This file is part of Rcpp.
//
Expand Down Expand Up @@ -331,7 +331,7 @@ class Vector :
}

inline iterator begin() { return cache.get() ; }
inline iterator end() { return cache.get() + static_cast<int>(size()) ; }
inline iterator end() { return cache.get() + size(); }
inline const_iterator begin() const{ return cache.get_const() ; }
inline const_iterator end() const{ return cache.get_const() + size() ; }
Copy link
Member Author

Choose a reason for hiding this comment

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

A related question is whether the two const interators on lines 336 and 338 should also get the cast.

Copy link
Member

Choose a reason for hiding this comment

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

It feels certainly wrong that there is a cast only there. What image did you use to reproduce the warnings?

Copy link
Member

Choose a reason for hiding this comment

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

Also, given that this was address in two steps (one, two), and this line was part of the first PR, is it possible that, after the second set of changes, the static_cast is not needed anymore? Because size() already returns R_xlen_t. (Specifically, after this change).

We should be fine just dropping the cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so too. We shuld be able to drop the cast.

I just installed clang++-17, at the time likely on Debian maybe using unstable. It is currently available in my default Ubuntu 24.04 setup.

There are other packages having issues with clang++-18 and now clang++-19 so we should maybe try those.

Copy link
Member Author

Choose a reason for hiding this comment

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

No issues with clang++-19 with the compiler flags that tickled this, either in installation or check. Here is installation (in a rocker/r-base container using Debian testing/unstable)

root@e47796620ac8:/work# install.r
* installing *source* package found in current working directory ...
* installing *source* package ‘Rcpp’ ...
** using staged installation
** libs
using C++ compiler: ‘Debian clang version 19.1.0 (4)’
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c api.cpp -o api.o
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c attributes.cpp -o attributes.o
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c barrier.cpp -o barrier.o
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c date.cpp -o date.o
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c module.cpp -o module.o
clang++-19  -I"/usr/share/R/include" -DNDEBUG -I../inst/include/      -fpic  -Wall -O3 -pedantic -Wconversion -Wno-sign-conversion   -c rcpp_init.cpp -o rcpp_init.o
clang++-19 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-z,relro -o Rcpp.so api.o attributes.o barrier.o date.o module.o rcpp_init.o -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/00LOCK-work/00new/Rcpp/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (Rcpp)
root@e47796620ac8:/work#

Copy link
Member Author

Choose a reason for hiding this comment

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

(To clarify, that was after I removed the cast.)

Copy link
Member Author

@eddelbuettel eddelbuettel Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've now pushed it. I will take the slow road here and let the current (2 1/2 day total !!) rev.dep test finish, fill-in missing build dependencies it uncovers and then re-run again with the cast removed.

inline const_iterator cbegin() const{ return cache.get_const() ; }
Expand Down
Loading