r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
199 stars 46 forks source link

Read-only matrix indexing operators and iterator methods are not const #234

Closed anthonynorth closed 2 years ago

anthonynorth commented 3 years ago

The index operator, call operator, begin and end methods should be const for a read-only matrix.

All statements in the below snippet are compile errors.

void matrix_const(const cpp11::integers_matrix& mat) {
  mat[0]; 
  mat[0][0];
  mat(0, 0);
  mat.begin();
  mat.end();
}
alyst commented 2 years ago

I haven't tried the updated matrix class yet, but now I feel I don't understand the general cpp11 classes design approach. I thought that cpp11 doesn't use const argument and method specifiers, but all the methods (begin/end/[] etc) are effectively read-only for classes declared in cpp11 namespace and read-write in cpp11::writable namespace. E.g. in cpp11::list_of the operator[] is not declared as const. Should then const be used everywhere for the methods of non-writable:: objects? But then if cpp11 is actually using const, what is the reason to have writable:: if we can have both const- and non-const method overloads in a single class?

jimhester commented 2 years ago

The regular non-writable classes are essentially equivalent to a string_view(), they provide a non-mutable read-only view of the data in an R object, so basically all the methods might as well be const.

Mutable classes require copying the data on construction, so having a more explicit difference than just constness was deemed worthwhile.

Basically I wanted people to default to using the read-only classes, and use the writable ones only where truly necessary.

Relying on user to specifying an object as const would have done the opposite, people would by default have been using a mutable class, which would then result in many more data copies in practice.

It is also not possible to have const and non-const constructors, which would mean we would have to defer copying until later. I wanted to avoid this as I find it easier to reason about copies when they are done eagerly at object construction.

alyst commented 2 years ago

@jimhester Thanks for the explanation, now I understand it better. So over time more methods of classes in non-::writable namespace would be declared as const?

jimhester commented 2 years ago

yeah, they probably should basically all be const, we just haven't gone through systematically to make it so.