Use if-constexpr more in Tensor. #16468
Conversation
| Assert(dim != 0, | ||
| ExcMessage("Cannot access an object of type Tensor<rank_,0,Number>")); | ||
| AssertIndexRange(i, dim); | ||
|
|
||
| return values[i]; |
There was a problem hiding this comment.
I see no constexpr in this function; given that the previous code appears to have been a workaround (introduced by #1573, see there for the message it tries to fix) to silence compiler warnings, what prevents these warnings now? Or don't we support the old compilers any more that would trigger the warnings?
There was a problem hiding this comment.
Good question. I didn't investigate the history when I wrote this patch. I'd say that if the CI checks come back clean, we should assume that compilers have learned not to warn.
(No constexpr -- I accidentally used the commit message I had used for a bigger patch that had test problems also for the case I here that I decided to break out into its own patch. I'll fix the SUBJECT line.)
There was a problem hiding this comment.
And just to be clear, we put the workaround in #1573 in more than 8 years ago. I don't think we still support many of the compilers we used at the time.
include/deal.II/base/tensor.h
Outdated
| typename numbers::NumberTraits<Number>::real_type s = | ||
| internal::NumberType< | ||
| typename numbers::NumberTraits<Number>::real_type>::value(0.0); | ||
| for (unsigned int i = 0; i < dim; ++i) | ||
| s += numbers::NumberTraits<Number>::abs_square(values[i]); |
There was a problem hiding this comment.
I'm usually preferring to peel off the first loop index because it avoids adding into zero (and compilers would need -ffast-math to avoid the instruction); in this case, it wouldn't even introduce more lines of code because we would write
| typename numbers::NumberTraits<Number>::real_type s = | |
| internal::NumberType< | |
| typename numbers::NumberTraits<Number>::real_type>::value(0.0); | |
| for (unsigned int i = 0; i < dim; ++i) | |
| s += numbers::NumberTraits<Number>::abs_square(values[i]); | |
| typename numbers::NumberTraits<Number>::real_type s = | |
| numbers::NumberTraits<Number>::abs_square(values[0]); | |
| for (unsigned int i = 1; i < dim; ++i) | |
| s += numbers::NumberTraits<Number>::abs_square(values[i]); |
The question that remains for me is if would trigger a warning for dim = 0. I do not feel particularly strongly about it, so either one is fine for me.
There was a problem hiding this comment.
This is a loop with compile-time bounds. It's almost certainly going to be unrolled. Don't you think the compiler can make the optimization itself? I think it's easier to read this way, but I also don't feel strongly about it.
There was a problem hiding this comment.
I changed my mind and did as you suggested. I handled the dim==0 case explicitly via an other if constexpr at the top.
| constexpr DEAL_II_HOST_DEVICE inline DEAL_II_ALWAYS_INLINE void | ||
| division_operator(Tensor<rank, dim, Number> (&t)[dim], | ||
| const OtherNumber &factor) | ||
| if constexpr (std::is_integral< |
There was a problem hiding this comment.
Oh, I do have a constexpr here -- the SUBJECT line wasn't totally wrong ;-)
In pursuit of #16465. I think this patch is best viewed side-by-side because the diffs show up poorly aligned.