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

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

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

zasdfgbnm
Copy link
Collaborator

No description provided.

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