Skip to content

Move opcheck to C++20 concepts for DynamicType lib#4109

Draft
zasdfgbnm wants to merge 18 commits intomainfrom
drop-opcheck
Draft

Move opcheck to C++20 concepts for DynamicType lib#4109
zasdfgbnm wants to merge 18 commits intomainfrom
drop-opcheck

Conversation

@zasdfgbnm
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 19, 2025

Review updated until commit 6098935

Description

  • Replaced opcheck with C++20 concepts for type checking.

  • Updated assignment and unary/binary operator definitions.

  • Simplified and modernized template metaprogramming.


Changes walkthrough 📝

Relevant files
Enhancement
assignment.cpp
Update assignment test cases                                                         

lib/dynamic_type/test/assignment.cpp

  • Updated macro TEST_ASSIGN_OP to use new assignment operators.
  • Replaced += with assign_op in test cases.
  • +14/-14 
    dynamic_type.h
    Replace opcheck with C++20 concepts                                           

    lib/dynamic_type/src/dynamic_type/dynamic_type.h

  • Replaced opcheck with C++20 requires expressions for type checking.
  • Updated template metaprogramming to use concepts.
  • Simplified and modernized operator definitions.
  • +172/-198

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The use of requires in the can_cast_to function may not handle all cases correctly, especially if the type T cannot be constructed from source. This could lead to compilation errors or unexpected behavior.

    return requires(typename decltype(t)::type source) {
      static_cast<T>(source);
    };
    Code Clarity

    The use of requires in the operator<< function could be improved for clarity. The lambda function inside any_check is complex and could be simplified or extracted into a named function for better readability.

    std::ostream& operator<<(std::ostream& os, const DT& dt)
        requires(is_dynamic_type_v<DT>&& any_check(
            [](auto x) {
              using T = typename decltype(x)::type;
              return requires(std::ostream & os, T t) {
                {
                  os << t
                } -> std::same_as<std::ostream&>;
              };
            },
    Code Clarity

    The DEFINE_ASSIGNMENT_OP macro could be simplified by removing the requires clause and using a more straightforward approach to check for the existence of the operator. This would make the code easier to understand and maintain.

    #define DEFINE_ASSIGNMENT_OP(op, assign_op)                      \
      template <typename DT, typename T>                             \
      requires is_dynamic_type_v<DT>&& requires(DT d, T t) {         \
        d op t;                                                      \

    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.

    1 participant