Skip to content

Ensure that data_p_ is typed with the underlying type of data_ #267

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
May 17, 2023
Merged

Ensure that data_p_ is typed with the underlying type of data_ #267

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
7 changes: 7 additions & 0 deletions inst/include/cpp11/R.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ constexpr R_xlen_t operator"" _xl(unsigned long long int value) { return value;

} // namespace literals

namespace traits {
template <typename T>
struct get_underlying_type {
using type = T;
};
} // namespace traits
Copy link
Member Author

Choose a reason for hiding this comment

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

The traits namespace may be overkill, I'm not sure.

I have seen this idea used elsewhere though. One of those places is Rcpp:
https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/traits/storage_type.h


template <typename T>
inline T na();

Expand Down
3 changes: 2 additions & 1 deletion inst/include/cpp11/doubles.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ inline double r_vector<double>::operator[](const R_xlen_t pos) const {
}

template <>
inline double* r_vector<double>::get_p(bool is_altrep, SEXP data) {
inline typename r_vector<double>::underlying_type* r_vector<double>::get_p(bool is_altrep,
SEXP data) {
if (is_altrep) {
return nullptr;
} else {
Expand Down
3 changes: 2 additions & 1 deletion inst/include/cpp11/integers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ inline int r_vector<int>::operator[](const R_xlen_t pos) const {
}

template <>
inline int* r_vector<int>::get_p(bool is_altrep, SEXP data) {
inline typename r_vector<int>::underlying_type* r_vector<int>::get_p(bool is_altrep,
SEXP data) {
if (is_altrep) {
return nullptr;
} else {
Expand Down
2 changes: 1 addition & 1 deletion inst/include/cpp11/list.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ inline SEXP r_vector<SEXP>::operator[](const r_string& name) const {
}

template <>
inline SEXP* r_vector<SEXP>::get_p(bool, SEXP) {
inline typename r_vector<SEXP>::underlying_type* r_vector<SEXP>::get_p(bool, SEXP) {
return nullptr;
}

Expand Down
15 changes: 8 additions & 7 deletions inst/include/cpp11/logicals.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,23 @@ inline SEXP r_vector<r_bool>::valid_type(SEXP data) {

template <>
inline r_bool r_vector<r_bool>::operator[](const R_xlen_t pos) const {
return is_altrep_ ? static_cast<r_bool>(LOGICAL_ELT(data_, pos)) : data_p_[pos];
return is_altrep_ ? LOGICAL_ELT(data_, pos) : data_p_[pos];
Copy link
Member Author

Choose a reason for hiding this comment

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

LOGICAL_ELT() and data_p_[] now return the same type (an int). So in theory the right thing to do would be to wrap the whole ternary condition in static_cast<r_bool>, but I'm relying on the implicit cast to r_bool instead - this seems to be what we do elsewhere.

I am not sure what best practice is here, rely on implicit casts to the return value, or be explicit with static_cast<r_bool>? I think I'd rather see the explicit cast, so there is no guessing involved. I'll probably do a follow up PR to try this out everywhere.

Copy link
Member Author

@DavisVaughan DavisVaughan Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently it is a Core Guideline to avoid implicit conversions where possible http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c164-avoid-implicit-conversion-operators

I resonate strongly with this! However, I will leave this as is for now since we use implicit casts in many cases already.

It would be even more apparent that we should be using an explicit static_cast<r_bool> if we had made the "r_bool from int" constructor explicit, i.e. explicit r_bool(int value) in r_bool.hpp, which would have forced us to use a static cast here. I feel like this would be better practice, maybe we should look into this. (Note that I did this for explicit r_complex(Rcomplex value) in the complex PR and really like the safety of it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that explicit is better.

}

template <>
inline r_bool* r_vector<r_bool>::get_p(bool is_altrep, SEXP data) {
inline typename r_vector<r_bool>::underlying_type* r_vector<r_bool>::get_p(bool is_altrep,
SEXP data) {
if (is_altrep) {
return nullptr;
} else {
return reinterpret_cast<r_bool*>(LOGICAL(data));
return LOGICAL(data);
Comment on lines -40 to +41
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the motivating example. get_p() now has a return type of int* for logicals, which is exactly what LOGICAL() returns, so no need for the cast

}
}

template <>
inline void r_vector<r_bool>::const_iterator::fill_buf(R_xlen_t pos) {
length_ = std::min(64_xl, data_->size() - pos);
LOGICAL_GET_REGION(data_->data_, pos, length_, reinterpret_cast<int*>(buf_.data()));
LOGICAL_GET_REGION(data_->data_, pos, length_, buf_.data());
Comment on lines -47 to +48
Copy link
Member Author

Choose a reason for hiding this comment

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

buf_ now has type int* here already, so no need to cast it

block_start_ = pos;
}

Expand All @@ -66,7 +67,7 @@ inline typename r_vector<r_bool>::proxy& r_vector<r_bool>::proxy::operator=(
template <>
inline r_vector<r_bool>::proxy::operator r_bool() const {
if (p_ == nullptr) {
return static_cast<r_bool>(LOGICAL_ELT(data_, index_));
return LOGICAL_ELT(data_, index_);
} else {
return *p_;
}
Expand Down Expand Up @@ -100,7 +101,7 @@ inline r_vector<r_bool>::r_vector(std::initializer_list<named_arg> il)
++n_protected;
auto it = il.begin();
for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
data_p_[i] = static_cast<r_bool>(LOGICAL_ELT(it->value(), 0));
data_p_[i] = LOGICAL_ELT(it->value(), 0);
SET_STRING_ELT(names, i, Rf_mkCharCE(it->name(), CE_UTF8));
}
UNPROTECT(n_protected);
Expand All @@ -121,7 +122,7 @@ inline void r_vector<r_bool>::reserve(R_xlen_t new_capacity) {

preserved.release(old_protect);

data_p_ = reinterpret_cast<r_bool*>(LOGICAL(data_));
data_p_ = LOGICAL(data_);
capacity_ = new_capacity;
}

Expand Down
7 changes: 7 additions & 0 deletions inst/include/cpp11/r_bool.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,11 @@ inline r_bool na() {
return NA_LOGICAL;
}

namespace traits {
template <>
struct get_underlying_type<r_bool> {
using type = int;
};
} // namespace traits

} // namespace cpp11
7 changes: 7 additions & 0 deletions inst/include/cpp11/r_string.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,11 @@ inline r_string na() {
return NA_STRING;
}

namespace traits {
template <>
struct get_underlying_type<r_string> {
using type = SEXP;
};
} // namespace traits

} // namespace cpp11
26 changes: 17 additions & 9 deletions inst/include/cpp11/r_vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class r_vector {
typedef T* pointer;
typedef T& reference;

using underlying_type = typename traits::get_underlying_type<T>::type;

r_vector() noexcept = default;

r_vector(SEXP data);
Expand Down Expand Up @@ -173,7 +175,7 @@ class r_vector {
void fill_buf(R_xlen_t pos);

R_xlen_t pos_;
std::array<T, 64 * 64> buf_;
std::array<underlying_type, 64 * 64> buf_;
R_xlen_t block_start_ = 0;
R_xlen_t length_ = 0;
};
Expand All @@ -193,10 +195,10 @@ class r_vector {
SEXP data_ = R_NilValue;
SEXP protect_ = R_NilValue;
bool is_altrep_ = false;
T* data_p_ = nullptr;
underlying_type* data_p_ = nullptr;
R_xlen_t length_ = 0;

static T* get_p(bool is_altrep, SEXP data);
static underlying_type* get_p(bool is_altrep, SEXP data);

static SEXP valid_type(SEXP data);

Expand All @@ -216,6 +218,8 @@ class r_vector : public cpp11::r_vector<T> {

// These are necessary because type names are not directly accessible in
// template inheritance
using typename cpp11::r_vector<T>::underlying_type;

using cpp11::r_vector<T>::data_;
using cpp11::r_vector<T>::data_p_;
using cpp11::r_vector<T>::is_altrep_;
Expand All @@ -228,11 +232,11 @@ class r_vector : public cpp11::r_vector<T> {
private:
const SEXP data_;
const R_xlen_t index_;
T* const p_;
underlying_type* const p_;
bool is_altrep_;

public:
proxy(SEXP data, const R_xlen_t index, T* const p, bool is_altrep);
proxy(SEXP data, const R_xlen_t index, underlying_type* const p, bool is_altrep);

proxy& operator=(const T& rhs);
proxy& operator+=(const T& rhs);
Expand Down Expand Up @@ -572,9 +576,9 @@ inline typename cpp11::r_vector<T>::const_iterator cpp11::r_vector<T>::find(
template <typename T>
inline T r_vector<T>::const_iterator::operator*() const {
if (data_->is_altrep()) {
return buf_[pos_ - block_start_];
return static_cast<T>(buf_[pos_ - block_start_]);
} else {
return data_->data_p_[pos_];
return static_cast<T>(data_->data_p_[pos_]);
Comment on lines -575 to +581
Copy link
Member Author

Choose a reason for hiding this comment

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

We probably don't need these, the implicit casts to T will kick in as needed (for T = r_bool and T = uint8_t) but it feels like good practice to be explicit?

}
}

Expand All @@ -598,13 +602,17 @@ inline T r_vector<T>::operator[](size_type pos) const {
namespace writable {

template <typename T>
r_vector<T>::proxy::proxy(SEXP data, const R_xlen_t index, T* const p, bool is_altrep)
r_vector<T>::proxy::proxy(SEXP data, const R_xlen_t index,
typename r_vector<T>::underlying_type* const p, bool is_altrep)
: data_(data), index_(index), p_(p), is_altrep_(is_altrep) {}

template <typename T>
inline typename r_vector<T>::proxy r_vector<T>::iterator::operator*() const {
if (data_.is_altrep()) {
return proxy(data_.data(), pos_, const_cast<T*>(&buf_[pos_ - block_start_]), true);
return proxy(
data_.data(), pos_,
const_cast<typename r_vector<T>::underlying_type*>(&buf_[pos_ - block_start_]),
true);
} else {
return proxy(data_.data(), pos_,
data_.data_p_ != nullptr ? &data_.data_p_[pos_] : nullptr, false);
Expand Down
17 changes: 12 additions & 5 deletions inst/include/cpp11/raws.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@

namespace cpp11 {

namespace traits {
template <>
struct get_underlying_type<uint8_t> {
using type = Rbyte;
};
} // namespace traits

template <>
inline SEXP r_vector<uint8_t>::valid_type(SEXP data) {
if (data == nullptr) {
Expand All @@ -34,20 +41,20 @@ inline uint8_t r_vector<uint8_t>::operator[](const R_xlen_t pos) const {
}

template <>
inline uint8_t* r_vector<uint8_t>::get_p(bool is_altrep, SEXP data) {
inline typename r_vector<uint8_t>::underlying_type* r_vector<uint8_t>::get_p(
bool is_altrep, SEXP data) {
if (is_altrep) {
return nullptr;
} else {
return reinterpret_cast<uint8_t*>(RAW(data));
return RAW(data);
}
}

template <>
inline void r_vector<uint8_t>::const_iterator::fill_buf(R_xlen_t pos) {
using namespace cpp11::literals;
length_ = std::min(64_xl, data_->size() - pos);
unwind_protect(
[&] { RAW_GET_REGION(data_->data_, pos, length_, (uint8_t*)buf_.data()); });
unwind_protect([&] { RAW_GET_REGION(data_->data_, pos, length_, buf_.data()); });
block_start_ = pos;
}

Expand Down Expand Up @@ -124,7 +131,7 @@ inline void r_vector<uint8_t>::reserve(R_xlen_t new_capacity) {
protect_ = preserved.insert(data_);
preserved.release(old_protect);

data_p_ = reinterpret_cast<uint8_t*>(RAW(data_));
data_p_ = RAW(data_);
capacity_ = new_capacity;
}

Expand Down
3 changes: 2 additions & 1 deletion inst/include/cpp11/strings.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ inline r_string r_vector<r_string>::operator[](const R_xlen_t pos) const {
}

template <>
inline r_string* r_vector<r_string>::get_p(bool, SEXP) {
inline typename r_vector<r_string>::underlying_type* r_vector<r_string>::get_p(bool,
SEXP) {
return nullptr;
}

Expand Down