-
Notifications
You must be signed in to change notification settings - Fork 9
added equal assignment operator Array<T>::operator=(ArrayView<T const> &) #266
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
Conversation
corbett5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd either add a test or at least put in a TODO or issue that we should add a test.
| using IndexType = INDEX_TYPE; | ||
|
|
||
| /// The type when the data type is const. | ||
| using ViewTypeConst = ArrayView< std::remove_const_t< T > const, NDIM, USD, INDEX_TYPE, BUFFER_TYPE >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious but is the std::remove_const necessary? I thought you don't have problems with duplicate const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put in ArrayVIew<T>::operator=( ArrayView<T> const&), ArrayVIew<T>::operator=( ViewTypeConst const &), but then thought better of it. I felt it was a little unclear what the intent of the operator is, and whether it would be a source of bugs. I did put in the std::remove_const because I was getting some errors in the using, but now I am not sure I was this is what fixed the problem I was having. Also:
https://stackoverflow.com/questions/5781222/duplicate-const-qualifier-allowed-in-c-but-not-in-c
but when I remove the std::remove_const it compiles fine now. I could remove it, but I feel it is better to be explicit about it...not that this is valid reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but then again, if I do something like this:
array1d<double> a;
arrayView1d<double const> aView = a;
arrayView1d<double const>::ViewTypeConst anotherView = a;
std::cout<<LvArray::system::demangle(typeid(anotherView).name())<<std::endl;
it outputs:
LvArray::ArrayView<double const, 1, 0, int, LvArray::ChaiBuffer>
So it is dropping the redundant const. If you dry to do:
double const const a = 1.0;
you get:
/usr/WS2/settgast/Codes/geosx/GEOSX_wave/src/main/main.cpp:34:14: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier]
double const const aScalar = 1.0;
so looks like it is illegal, but the compiler drops the second const when it is an alias??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in http://eel.is/c++draft/dcl.type.cv#1 this sentence
Each cv-qualifier shall appear at most once in a cv-qualifier-seq.
refers to language grammar, i.e. what goes into text of a program, so it prohibits literally typing const const anywhere. But then
Redundant cv-qualifications are ignored.
[Note [2]: For example, these could be introduced by typedefs. — end note]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put in
ArrayVIew<T>::operator=( ArrayView<T> const&),ArrayVIew<T>::operator=( ViewTypeConst const &), but then thought better of it
We rely on the shallow copy semantics of ArrayView in at least once place where we construct the element accessors. Plus it'd be weird to have the copy constructor do something so different from the assignment operator.
Title says it all.