Cleanup the FieldVector<K,1>

Summary

This MR simplified/cleansup/improves the FieldVector<K,1> without reducing its functionality.

There are several issues this MR addresses, especially with the constructors from scalar and from DenseVector due to the complicated enable_if constraints. See also !1040 (comment 85553)

changed title from Draft: Cleanup the FieldVector<1> constructors to Draft: Cleanup the FieldVector<K,1>

changed the description

mentioned in merge request !1040

added 5 commits

• 570f2eaa - Rewrite the FieldVector<1> constructors and assignment operators
• 8c726966 - Make the member functions of FieldVector<1> constexpr
• e61fdaf0 - Make the member functions of FieldVector<1> noexcept
• 3c5f6040 - Unify formatting for FieldVector<1>
• 66f78a79 - Use reference and const_reference as return type

Compare with previous version

added 1 commit

• 2afe6063 - Restruct densevector constructor and assignment operators to those with correct size

Compare with previous version

I have been wondering whether implementing the size-1 case as a specialization is a good idea, because it is challenging to keep that specialization consistent with the general case.

The size-1 case is basically the general case with a few extra methods. Wouldn't it be easier to remove the specialization and SFINAE-enable the extra methods directly in the unspecialized vector class?

Yes, that would probably be the case. (This is how the rank-0 case is handled in dune-tensor) But this may be a more drastic change than just cleaning up the constructors

BTW. It is not so easy as you might think, due to the complicated expectations of the implicit conversion to the underlying scalar type in case of FieldVector<K,1>.

For example, if we implement the implicit conversion with enable-if constraints:

template <int S = SIZE,
  std::enable_if_t<(S == 1), int> = 0>
constexpr operator K () const noexcept { return _data[0]; }

I get an "error: assigning to 'int' from incompatible type 'FieldVector<float, 1>'".

If instead I use

constexpr operator K () const noexcept requires(SIZE == 1) { return _data[0]; }

it works (but we do not have c++20 concepts yet). I could also remove any constraint and it works, but then we get implicit conversions to the 0th element for all FieldVectors what we probably don't want.

If I add another overload for the reference:

constexpr operator K& () noexcept requires(SIZE == 1)  { return _data[0]; }

I get problems with GMPField: "error: use of overloaded operator '=' is ambiguous (with operand types 'Dune::GMPField<128>' and 'FieldVector<T, 1>' (aka 'FieldVector<GMPField<128U>, 1>')", while it is working in a separate class specialization.

See !1401 for an experiment

I know it's not simple -- I tried it myself. :-)

I do like the C++20 replacement -- much more readable.

But at the same time I'm thinking we should just deprecate these strange obsolete methods and get rid of them eventually.

But at the same time I'm thinking we should just deprecate these strange obsolete methods and get rid of them eventually.

This is something you should do, not me. I don't want to work on all the consequences in probably all dune modules.

I have been wondering whether implementing the size-1 case as a specialization is a good idea, because it is challenging to keep that specialization consistent with the general case.

The size-1 case is basically the general case with a few extra methods. Wouldn't it be easier to remove the specialization and SFINAE-enable the extra methods directly in the unspecialized vector class?

Now that I have invested time to follow your comment on my current MR and have rewritten the whole FieldVector class in !1401 I would like to ask you to have a look at the outcome and compare it to the current proposal. What is the better solution?

added 1 commit

• e71a17f4 - Remove constexpr from operator[] due to DUNE_ASSERT_BOUND exception

Compare with previous version

marked this merge request as ready

changed the description

mentioned in merge request !1401

There is a typo in the commit message for this commit.

This can be easily fixed. But I would do this only at the very end before merging to not trigger the CI for multiple non-modifying changes.

I'd say this is the very end: go fix the commit message and then merge.

added 49 commits

• e71a17f4...5368e280 - 42 commits from branch master
• 146abbef - Rewrite the FieldVector<1> constructors and assignment operators
• 52effd26 - Make the member functions of FieldVector<1> constexpr
• 2d63b5d0 - Make the member functions of FieldVector<1> noexcept
• e7ebf38d - Unify formatting for FieldVector<1>
• ad22f069 - Use reference and const_reference as return type
• aba0937f - Restrict DenseVector constructor and assignment operators to those with correct size
• 1c0951d6 - Remove constexpr from operator[] due to DUNE_ASSERT_BOUND exception

Compare with previous version

merged

mentioned in commit 7444b40d

mentioned in merge request !1407 (merged)