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

SymmetricTensor access returns const reference #14839

Merged

Conversation

sebproell
Copy link
Contributor

Interestingly, the following compiled:

const SymmetricTensor<2, dim, VectorizedArray<double>> tensor{};
tensor[0][0] = 1.0;

The assignement is performed on a temporary copy returned from the accessor but I obviously wanted to modify the actual tensor. Imo, the correct behavior would be to get a compiler error that tensor is const (in this case by accident).

I don't know whether the decision to return a value rather a reference was intentional. Unless it enables some optimizations(?), I don't see why the const access to a tensor should return something different than a const reference.

@drwells
Copy link
Member

drwells commented Mar 3, 2023

The original type seems wrong - if it should be a value then it should be const Number. I'm not sure if there's any advantage to doing this one way or the other.

/rebuild

using reference = Number;
using reference = const Number &;
Copy link
Member

Choose a reason for hiding this comment

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

I guess the question is whether we want to expose the underlying memory address (as with a reference) or not (then, the right type would be const Number;). I agree with the current patch.

@kronbichler
Copy link
Member

This is clearly forward progress, let us merge this now.

@kronbichler kronbichler merged commit cd1cf88 into dealii:master Mar 3, 2023
@bangerth
Copy link
Member

bangerth commented Mar 4, 2023

What an interesting bug! The original design was for tensors with double underlying scalar types. There, if the function just returns a double, it is an rvalue to which you can't assign anything: an rvalue of non-class type can't appear on the left hand side of an assignment.

But if it is a class type like you have here, one can by default. I bet that we have that bug in any number of places where we access elements of arrays of some sort and the const version of operator[] returns the object by value.

There is a way to disallow this, starting in C++11, see https://stackoverflow.com/questions/26809789/how-to-disallow-assigning-to-a-temporary-object-and-implicit-casting-from-other . In essence, we would have to add

VectorizedArray& operator=(const VectorizedArray &) && = delete;

to class VectorizedArray -- note the && at the end of the declaration. I am pretty sure that we actually want to disallow this kind of thing and so if someone wanted to give a patch to this effect a try, I would very much be in favor of it. @sebproell: Want to give this a try?

@sebproell sebproell deleted the symmetric-tensor-const-accessor branch March 4, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants