Draft: Remove the FieldVector<K,1> specialization
Summary
This MR removes the separate specialization of FieldVector<K,1>
as a scalar type. Instead some functions are added to the primary template constrained to be only valid of SIZE == 1
case. Additionally, the interaction with scalar (field) types is being improved by allowing any type that is counted as IsNumber
. Some alternatives were considered. Due to the implicit conversion operator of FieldVector<K,1>
to const K&
, several options lead to ambiguities.
Instead of using std::enable_if
to constrain the functions to specific cases, this MR uses c++20 concepts and a requires
clause. This makes the code more readable and the error messages better explaining what the problem is.
ToDo
-
Requires an updated compiler toolchain with c++20 enabled. -
Extract some changes into separate MRs, e.g. !1492 (merged), !1493 (merged) -
Include changes from !1472 (merged) that makes DUNE_THROW usable in constexpr
Merge request reports
Activity
mentioned in merge request !1387 (merged)
- Resolved by Simon Praetorius
I like this. Not as much as getting rid of the size-1 special case altogether, but that would require a larger democratic process and consensus.
It seems your patch looks scarier than it actually is, because you are apparently doing some additional cleanup. Adding
constexpr
andnorthrow
, e.g., should be uncontroversal. If that was presented as a separate MR it could probably go in right away.Also, you seem be tightening some of the rules of what can be assigned to what. That, too, appears to be orthogonal to the size-1 specialization.
371 371 //===== forward methods to container 372 372 static constexpr size_type size () noexcept { return 1; } 373 373 374 constexpr reference operator[] ([[maybe_unused]] size_type i) 374 reference operator[] ([[maybe_unused]] size_type i) 375 375 { 376 376 DUNE_ASSERT_BOUNDS(i == 0); Because this MR was not supposed to change the existing behavior.
Also, the
DUNE_ASSERT_BOUNDS
was original invented to do these bounds checks. Changing this in just one place here, would introduce another inconsistency in the dune-common code base. We should decide on doing this everywhere, or change the implementation ofDUNE_ASSERT_BOUNDS
, see also the discussion in !1388 (closed) and #110.
- Resolved by Simon Praetorius
@simon.praetorius , can you please rebase this MR?
added 48 commits
-
54b851a4...73c7034c - 47 commits from branch
master
- 86b12cb1 - Remove the FieldVector<K,1> specialization
-
54b851a4...73c7034c - 47 commits from branch
- Resolved by Simon Praetorius
My impression is that this patch makes the code much shorter and simpler.
added 1 commit
- fbd7ddf5 - Make the generic specializations of DenseMatrixAssigner more safe
mentioned in issue staging/dune-functions#88 (closed)
mentioned in issue #373
added 67 commits
-
26633d1d...7958bd7e - 61 commits from branch
master
- 8b429b93 - Remove the FieldVector<K,1> specialization
- c2f02c6d - Make the generic specializations of DenseMatrixAssigner more safe
- e5bbbf1c - Fix assignment of matrices traits
- 902c5a24 - Use type properties defined before usage
- e9bd00a5 - Merge branch 'issue/fieldvector-1-constructors-2' of...
- 7e63cb48 - Rewrite the FieldVector using concepts
Toggle commit list-
26633d1d...7958bd7e - 61 commits from branch
added 1 commit
- cd53eb21 - Remove operator== for FieldVector in favor of DenseVector overload
added 2 commits
requested review from @oliver.sander
assigned to @simon.praetorius
- Resolved by Simon Praetorius
- dune/common/concepts/scalar.hh 0 → 100644
1 // SPDX-FileCopyrightText: Copyright © DUNE Project contributors, see file LICENSE.md in module root 2 // SPDX-License-Identifier: LicenseRef-GPL-2.0-only-with-DUNE-exception 3 // -*- tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- 4 // vi: set et ts=4 sw=2 sts=2: 5 #ifndef DUNE_COMMON_CONCEPTS_SCALAR_HH 6 #define DUNE_COMMON_CONCEPTS_SCALAR_HH 7 8 // check whether c++20 concept can be used 9 #if __has_include(<version>) && __has_include(<concepts>) changed this line in version 14 of the diff
- dune/common/concepts/scalar.hh 0 → 100644
13 #define DUNE_ENABLE_CONCEPTS 1 14 #endif 15 #endif 16 #endif 17 18 #if DUNE_ENABLE_CONCEPTS 19 20 #include <complex> 21 #include <dune/common/typetraits.hh> 22 23 namespace Dune::Concept { 24 25 template <class N> 26 concept Scalar = Dune::IsNumber<N>::value; 27 28 static_assert(Scalar<short>); changed this line in version 14 of the diff
77 78 } 78 79 }; 79 80 81 template <class A, class B> 82 using MatrixElementsAssignable = decltype( 83 *std::begin(*std::declval<typename A::iterator>()) = *std::begin(*std::declval<typename B::const_iterator>())); 84 80 85 template< class DenseMatrix, class RHS > 81 class DenseMatrixAssigner< DenseMatrix, RHS, std::enable_if_t< !std::is_same< typename RHS::const_iterator, void >::value 82 && std::is_convertible< typename RHS::const_iterator::value_type, typename DenseMatrix::iterator::value_type >::value > > 86 class DenseMatrixAssigner< DenseMatrix, RHS, 87 std::enable_if_t<Dune::Std::is_detected_v<MatrixElementsAssignable,DenseMatrix,RHS>>> changed this line in version 11 of the diff
mentioned in merge request !1433
mentioned in merge request !1459 (merged)
added 41 commits
-
04434c4f...4228b538 - 27 commits from branch
master
- 4228b538...46ca67b8 - 4 earlier commits
- 5b8cd4e2 - Make the generic specializations of DenseMatrixAssigner more safe
- f8d076a5 - Fix assignment of matrices traits
- 52f60f5b - Use type properties defined before usage
- 4ef887b8 - Rewrite the FieldVector using concepts
- 6d16e8db - Fix a typo
- a986f4b4 - Remove operator== for FieldVector in favor of DenseVector overload
- f871df20 - Replace assignable_from by is_assignable
- 280c38dd - Add scalar concept
- d1ea79dc - Improve the documentation and use scalar concept
- 2ea60605 - Improve DenseMatrixAssigner constraint
Toggle commit list-
04434c4f...4228b538 - 27 commits from branch
added 31 commits
-
2ea60605...89b80dc4 - 17 commits from branch
master
- 89b80dc4...a96be52a - 4 earlier commits
- 6bf40844 - Make the generic specializations of DenseMatrixAssigner more safe
- 5e121c3a - Fix assignment of matrices traits
- 8ea4bcc9 - Use type properties defined before usage
- a06ddb81 - Rewrite the FieldVector using concepts
- 0611ff3e - Fix a typo
- 567a10c1 - Remove operator== for FieldVector in favor of DenseVector overload
- 6f391a12 - Replace assignable_from by is_assignable
- 1bb6dbff - Add scalar concept
- 8076d8da - Improve the documentation and use scalar concept
- cb19ba27 - Improve DenseMatrixAssigner constraint
Toggle commit list-
2ea60605...89b80dc4 - 17 commits from branch
added 1 commit
- 8688d7a7 - Improve the documentation and add a fallback implementation for the std concepts
mentioned in merge request !1388 (closed)
added 60 commits
-
68ef4108...aa5e86be - 36 commits from branch
master
- aa5e86be...3d3b144b - 14 earlier commits
- 65b0ac4c - Add a concept for three-way comparison
- 349a6e91 - Move comparison concepts into std directory
- 730440d3 - Add tests for scalar concept
- 3fe1c7d0 - Fix include directory
- 339d77f9 - Add more concept backport implementations
- b8fdcc20 - Improve the documentation and add a fallback implementation for the std concepts
- cbfaa0cc - Remove an unnecessary include
- 06a7541a - Add an include in the test
- 12a864dd - Add missing semicolon
- 2ab4d1ff - Remove deleted constructor and assignment operator
Toggle commit list-
68ef4108...aa5e86be - 36 commits from branch
added 49 commits
-
2ab4d1ff...20ae67fb - 26 commits from branch
master
- 20ae67fb...575686f2 - 13 earlier commits
- 5fa54499 - Improve DenseMatrixAssigner constraint
- b25d7928 - Add a concept for three-way comparison
- 523ed239 - Move comparison concepts into std directory
- d58f2680 - Add tests for scalar concept
- cb80bc07 - Fix include directory
- b41c783a - Add more concept backport implementations
- 71f2ca5d - Improve the documentation and add a fallback implementation for the std concepts
- a57347f3 - Remove an unnecessary include
- 4ac1e58f - Remove deleted constructor and assignment operator
- 7c7a775a - Cleanup after rebase
Toggle commit list-
2ab4d1ff...20ae67fb - 26 commits from branch