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

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

Closed
anthonynorth opened this issue Aug 16, 2021 · 4 comments
Closed

Comments

@anthonynorth
Copy link

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
Copy link
Contributor

alyst commented Sep 29, 2021

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
Copy link
Member

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
Copy link
Contributor

alyst commented Sep 29, 2021

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants