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
Use a std::array instead of a C-style array for tensor elements. #16474
Conversation
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.
This looks good. I'm curious if we see any effect on compilation times or library sizes, since the tensor class is used everywhere. I'm looking forward to trying it.
ar &values; | ||
if constexpr (rank_ > 1) | ||
ar &values; | ||
else | ||
ar &boost::serialization::make_array(&values[0], dim); |
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.
Let me add here that we could have used the first case also for the std::array
in the base case (rank==1) but that I expose the array explicitly because I would like to replace the std::array
in the base case by a VectorizedArray
and I don't want to change the serialization output again at that point.
(VectorizedArray
does not have the infrastructure to serialize itself, but in any case, I can only use it when available and need to fall back to std::array
otherwise. This would lead to platform-dependent serialization output, which I can avoid by just describing the elements here as an explicit array.)
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 am looking forward to that - albeit I would note that we need to look out for some places, e.g. for what happens with Tensor<1, dim, VectorizedArray<double>>
. (It should just work fine, because VectorizedArray<VectorizedArray<double>>
is valid; nonetheless, we might need to have a closer look there.)
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.
Right. std::conditional
is what is necessary, and I know how to do it :-)
I checked the library size (admittedly, I have a few PRs from this afternoon in my difference, but nothing is really substantial ); the debug library went from 641.6 MB to 642.9 MB, the release library even decreased from 303.95 MB to 303.93 MB. So nothing worth worrying, 👍 for the improvement. |
This gets rid of the awkwardness of the C-style array which we need to make sure has size greater than zero. This changes all of the serialization output.
See #16465.