Skip to content
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

Feature/tags as template args #512

Merged
merged 12 commits into from
Mar 5, 2024
Merged

Feature/tags as template args #512

merged 12 commits into from
Mar 5, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Mar 4, 2024

replace bool template args with a named tag for comprehensibility

This is something that I've been looking into for a long time now. Dev work on #486 kept getting slower and slower because I was struggling to forward the true and false types. I ended up getting frustrating myself so much that I decided to go ahead with this change.

Example for illustration

template <bool sym>
struct A{
  RealValue<sym> a;
};

using SymA = A<true>;
using AsymA = A<false>;

becomes

template <symmetry_tag sym>
struct A{
  RealValue<sym> a;
};

using SymA = A<symmetric_t>;
using AsymA = A<asymmetric_t>;

This makes types like RealValue<symmetric_t> and functions get_output<asymmetric_t> much more comprehensible than existing RealValue<false> and get_output<false>, resp.

Example on datasets to really show why we need this

  • DataPointer uses is_const
  • but DataHandler uses data_mutable

this is confusing, because in one case true means const and in the other true means mutable

// dataset.hpp
template <bool is_const> class DataPointer {
  // ...
};

// dataset_handler.hpp
template <bool data_mutable, bool indptr_mutable>
    requires(data_mutable || !indptr_mutable)
class DataHandler {
  // ...
};

Introduced constructs

  • For every changed construct, 2 tag types are created, one representing the old true value, and one representing the old false value
  • A tag concept is created that matches on only those two tag types and nothing else. This is equivalent to the bool part of the template parameter declaration
  • A constexpr bool is created to access the true resp false value of the tag types
// tag types
struct symmetric_t{};
struct asymmetric_t{};

// tag concept
template <typename T> symmetry_tag = std::same_as<symmetric_t> || std::same_as<asymmetric_t>;

// tag value
template <symmetry_tag sym> constexpr bool is_symmetric = std::same_as<T, symmetric_t>;

// static asserts
static_assert(symmetry_tag<symmetric_t>);    // it is indeed a tag
static_assert(symmetry_tag<asymmetric_t>);   // it is indeed a tag

static_assert(is_symmetric<symmetric_t>);    // represents current 'true'
static_assert(!is_symmetric<asymmetric_t>);  // represents current 'false'

Changed constructs

  • bool sym for calculation symmetry and object symmetry
  • bool is_const for the data set
  • bool data_mutable for the data set handler
  • bool indptr_mutable for the data set handler
  • bool is_gen for load/gen

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
This reverts commit a8b8462.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers force-pushed the feature/tags-as-template-args branch from aea940b to d2c36cc Compare March 4, 2024 16:06
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Copy link
Member

@TonyXiang8787 TonyXiang8787 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the suggestions.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Copy link

sonarcloud bot commented Mar 5, 2024

Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tons of repeated changes, I can see why you were frustrated ;) One minor suggestion: why do we not already declare
using SymComplexValue = ComplexValue<symmetric_t>; using AsymComplexValue = ComplexValue<asymmetric_t>;
throughout the code? This is just one instance.

@mgovers mgovers enabled auto-merge March 5, 2024 10:48
@mgovers mgovers dismissed TonyXiang8787’s stale review March 5, 2024 10:49

all comments resolved, no new comments + nitish approved

@mgovers mgovers merged commit b21f231 into main Mar 5, 2024
27 checks passed
@mgovers mgovers deleted the feature/tags-as-template-args branch March 5, 2024 10:49
@mgovers
Copy link
Member Author

mgovers commented Mar 5, 2024

Tons of repeated changes, I can see why you were frustrated ;) One minor suggestion: why do we not already declare using SymComplexValue = ComplexValue<symmetric_t>; using AsymComplexValue = ComplexValue<asymmetric_t>; throughout the code? This is just one instance.

out of scope of this ticket. TBD in next refinement

@mgovers mgovers mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants