Skip to content

Conversation

@zasdfgbnm
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 19, 2025

Review updated until commit 0b741e5

Description

  • Replace macro-defined operators with concept-based versions

  • Simplify OperatorChecker using C++20 concepts

  • Remove unnecessary template specializations and macros


Changes walkthrough 📝

Relevant files
Enhancement
type_traits.h
Replace opcheck macros with C++20 concepts                             

lib/dynamic_type/src/dynamic_type/type_traits.h

  • Removed macro definitions for unary, unary suffix, and binary
    operators
  • Replaced macro definitions with concept-based versions
  • Simplified OperatorChecker using C++20 concepts
  • Removed unnecessary template specializations and helper structs
  • +51/-133

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new implementation uses requires expressions which are C++20 features. Ensure that the build system and all dependencies are compatible with C++20.

    constexpr bool operator=(OperatorChecker<T1>) const {
      return requires(T t, T1 t1) {
        t = t1;
      };
    }
    Performance Impact

    Evaluate the performance impact of replacing the old SFINAE-based approach with the new requires expressions. Ensure that the new implementation does not introduce any performance regressions.

      constexpr bool operator=(OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t = t1;
        };
      }
    
      template <typename... Ts>
      constexpr bool operator()(OperatorChecker<Ts>... args) const {
        return can_use_args<T, Ts...>;
      }
    
      template <typename T1>
      constexpr bool operator[](OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t[t1];
        };
      }
    
      constexpr auto operator->() const {
        if constexpr (requires(T t) { t.operator->(); }) {
          return static_cast<TrueType*>(nullptr);
        } else {
          return static_cast<FalseType*>(nullptr);
        }
      }
    
      template <typename T1>
      constexpr bool operator->*(OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t->*t1;
        };
      }
    
      template <typename T1>
      constexpr bool canCastTo(OperatorChecker<T1>) const {
        return requires(T t) {
          static_cast<T1>(t);
        };
      }
    
      template <typename T1>
      constexpr bool hasExplicitCastTo(OperatorChecker<T1>) const {
        return requires(T t) {
          &T::operator T1;
        };
      }
    };
    
    // Replace macro-defined operators with concept-based versions
    #define DEFINE_UNARY_OP(op)                         \
      template <typename T1>                            \
      constexpr bool operator op(OperatorChecker<T1>) { \
        return requires(T1 t) {                         \
          op t;                                         \
        };                                              \
      }
    
    #define DEFINE_UNARY_SUFFIX_OP(op)                       \
      template <typename T1>                                 \
      constexpr bool operator op(OperatorChecker<T1>, int) { \
        return requires(T1 t) {                              \
          t op;                                              \
        };                                                   \
      }
    
    #define DEFINE_BINARY_OP(op)                                             \
      template <typename T1, typename T2>                                    \
      constexpr bool operator op(OperatorChecker<T1>, OperatorChecker<T2>) { \
        return requires(T1 t1, T2 t2) {                                      \
          t1 op t2;                                                          \
        };                                                                   \
      }
    Code Clarity

    The new implementation uses requires expressions which might be less familiar to developers not using C++20. Ensure that the code is well-documented and that the transition to C++20 is justified by significant benefits.

      constexpr bool operator=(OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t = t1;
        };
      }
    
      template <typename... Ts>
      constexpr bool operator()(OperatorChecker<Ts>... args) const {
        return can_use_args<T, Ts...>;
      }
    
      template <typename T1>
      constexpr bool operator[](OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t[t1];
        };
      }
    
      constexpr auto operator->() const {
        if constexpr (requires(T t) { t.operator->(); }) {
          return static_cast<TrueType*>(nullptr);
        } else {
          return static_cast<FalseType*>(nullptr);
        }
      }
    
      template <typename T1>
      constexpr bool operator->*(OperatorChecker<T1>) const {
        return requires(T t, T1 t1) {
          t->*t1;
        };
      }
    
      template <typename T1>
      constexpr bool canCastTo(OperatorChecker<T1>) const {
        return requires(T t) {
          static_cast<T1>(t);
        };
      }
    
      template <typename T1>
      constexpr bool hasExplicitCastTo(OperatorChecker<T1>) const {
        return requires(T t) {
          &T::operator T1;
        };
      }
    };
    
    // Replace macro-defined operators with concept-based versions
    #define DEFINE_UNARY_OP(op)                         \
      template <typename T1>                            \
      constexpr bool operator op(OperatorChecker<T1>) { \
        return requires(T1 t) {                         \
          op t;                                         \
        };                                              \
      }
    
    #define DEFINE_UNARY_SUFFIX_OP(op)                       \
      template <typename T1>                                 \
      constexpr bool operator op(OperatorChecker<T1>, int) { \
        return requires(T1 t) {                              \
          t op;                                              \
        };                                                   \
      }
    
    #define DEFINE_BINARY_OP(op)                                             \
      template <typename T1, typename T2>                                    \
      constexpr bool operator op(OperatorChecker<T1>, OperatorChecker<T2>) { \
        return requires(T1 t1, T2 t2) {                                      \
          t1 op t2;                                                          \
        };                                                                   \
      }

    @zasdfgbnm zasdfgbnm changed the title Move some opcheck in DynamicType lib to C++20 Move opcheck in DynamicType lib to C++20 Mar 19, 2025
    @zasdfgbnm
    Copy link
    Collaborator Author

    !test

    @zasdfgbnm zasdfgbnm requested review from naoyam and wujingyue March 19, 2025 19:31
    @zasdfgbnm zasdfgbnm marked this pull request as ready for review March 19, 2025 19:31
    @zasdfgbnm
    Copy link
    Collaborator Author

    Looks like this does not work with clang, changing to draft again and debugging.

    @zasdfgbnm zasdfgbnm marked this pull request as draft March 19, 2025 21:19
    @wujingyue wujingyue removed their request for review March 19, 2025 22:24
    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.

    3 participants