Skip to content
Snippets Groups Projects

Draft: Remove the FieldVector<K,1> specialization

Open Simon Praetorius requested to merge issue/fieldvector-1-constructors-2 into master

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
Edited by Simon Praetorius

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Simon Praetorius mentioned in merge request !1387 (merged)

    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 and northrow, 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.

  • Oliver Sander
    Oliver Sander @oliver.sander started a thread on an outdated change in commit a505c20e
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);
  • Why not remove this macro if it is such a pain? I can't think of a situation where throwing an exception here would be of any use. In my view any out-of-bounds array access here is certainly a bug, and the program may as well terminate directly.

  • 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 of DUNE_ASSERT_BOUNDS, see also the discussion in !1388 (closed) and #110.

  • Please register or sign in to reply
  • Christoph Grüninger resolved all threads

    resolved all threads

  • Simon Praetorius added 48 commits

    added 48 commits

    Compare with previous version

  • added 1 commit

    • fbd7ddf5 - Make the generic specializations of DenseMatrixAssigner more safe

    Compare with previous version

  • added 1 commit

    • 0fc3d48d - Fix assignment of matrices traits

    Compare with previous version

  • Simon Praetorius mentioned in issue #373

    mentioned in issue #373

  • added 1 commit

    • 26633d1d - Use type properties defined before usage

    Compare with previous version

  • Simon Praetorius added 67 commits

    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

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • cd53eb21 - Remove operator== for FieldVector in favor of DenseVector overload

    Compare with previous version

  • added 1 commit

    • d8449a35 - Replace assignable_from by is_assignable

    Compare with previous version

  • added 2 commits

    • f1000a26 - Add scalar concept
    • 57952bae - Improve the documentation and use scalar concept

    Compare with previous version

  • Simon Praetorius marked this merge request as ready

    marked this merge request as ready

  • requested review from @oliver.sander

  • Simon Praetorius changed the description

    changed the description

  • 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>)
  • 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>);
  • 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>>>
  • Simon Praetorius mentioned in merge request !1433

    mentioned in merge request !1433

  • added 1 commit

    • 04434c4f - Improve DenseMatrixAssigner constraint

    Compare with previous version

  • Simon Praetorius mentioned in merge request !1459 (merged)

    mentioned in merge request !1459 (merged)

  • Simon Praetorius added 41 commits

    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

    Compare with previous version

  • Simon Praetorius marked the checklist item Requires an updated compiler toolchain with c++20 enabled. as completed

    marked the checklist item Requires an updated compiler toolchain with c++20 enabled. as completed

  • Simon Praetorius added 31 commits

    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

    Compare with previous version

  • added 3 commits

    • 61712a72 - Add a concept for three-way comparison
    • d10009b1 - Move comparison concepts into std directory
    • 67404983 - Add tests for scalar concept

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • d6c6aa24 - Add more concept backport implementations

    Compare with previous version

  • added 1 commit

    • 8688d7a7 - Improve the documentation and add a fallback implementation for the std concepts

    Compare with previous version

  • added 1 commit

    • 3ce8e3f4 - Remove an unnecessary include

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Simon Praetorius mentioned in merge request !1388 (closed)

    mentioned in merge request !1388 (closed)

  • Simon Praetorius added 60 commits

    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

    Compare with previous version

  • Simon Praetorius changed the description

    changed the description

  • Simon Praetorius changed the description

    changed the description

  • Simon Praetorius marked this merge request as draft

    marked this merge request as draft

  • Simon Praetorius changed the description

    changed the description

  • Simon Praetorius added 49 commits

    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

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 8e8b2753 - Cleanup FieldVector code after rebase

    Compare with previous version

  • added 1 commit

    • 282ef168 - Cleanup FieldVector code after rebase

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading