Fix type error in `no_duplicate_field_names`

[lixin-wei]

Fixes the following compile error:

external/com_github_getml_reflect_cpp/include/rfl/internal/no_duplicate_field_names.hpp:16:26: error: ambiguous overload for 'operator==' (operand types are 'int' and 'const std::tuple_size<std::tuple<rfl::Field<rfl::internal::StringLiteral<12>{std::array<char, 11>{"op_history"}}, const elixir::utils::RingArray<double>*>, rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 4>{"sum"}}, const double*> > >')
   16 |         if constexpr (_i == num_fields) {

Seems num_fields is set to std::tuple_size by mistake. It should be std::tuple_size_v

[liuzicheng1987]

Could you tell me what compiler you are using there?

I have been using std::tuple_size<...>{} all the time and it compiled fine in gcc, clang and MSVC. (This function is being used in practically every test.)

But you are right, std::tuple_size<...>{} is not guaranteed to be constexpr:

https://en.cppreference.com/w/cpp/utility/tuple/tuple_size

I will merge the PR, for sure, I am just curious about this.

[ChemiCalChems]

It is constexpr, though. The problem is, as the compiler error states, there is no conversion path from std::tuple_size to int, since what you actually want is the value variable inherited from std::integral_constant.

My point is, like in many Standard library typetraits, what you get is not the value itself from the type, but a type containing the wanted value, and then a _v static constexpr variable associated to it, that's the problem here. This PR should be merged ASAP.

[lixin-wei]

my compiler is gcc12.3

[liuzicheng1987]

OK, merged. I have looked through the codebase. There appear to be a handful of places where I use std::tuple_size{}. I will fix that, too. I still think it's weird, that it worked for me, though (GCC 11.4, clang 17.0).

[ChemiCalChems]

The only reason it worked for you is because those paths were never crossed. He might have triggered that code path in some way, but it shouldn't have worked for any of us, we just got lucky, I think.

[liuzicheng1987]

@lixin-wei , thank you for pointing this out and your contribution.

[liuzicheng1987]

@ChemiCalChems , I don't think this could be the explanation...include/rfl/internal/no_duplicate_field_names.hpp:16:26 is used inside NamedTuple, which means that it is used in practically every single test we have...you cannot serialize or deserialize a single struct without this. This is probably one of the most covered functions in our codebase.

[lixin-wei]

std::integral_constant do have an implict conversation operator.

https://en.cppreference.com/w/cpp/types/integral_constant

but I don't know why it doesn't work here...

[ChemiCalChems]

Maybe too many conversions in a row, from std::tuple_size to std::size_t to int, but after reading cppreference I'm not so sure. Implicit conversions have always been sketchy...

[ChemiCalChems]

@liuzicheng1987 I went crazy before reading the code better, I have no clue either, quite strange. I still think we should express these sort of contracts are requires clauses for the whole class template rather than repeating ourselves in every constructor, I don't know what you think about that. Let's follow up on it if you want.

@lixin-wei Could you post the code that originally caused the error? Thanks!

[lixin-wei]

@liuzicheng1987 @ChemiCalChems I dug the error futher and find the root casue: I've some overloads of operator==, which cause this error. The following is the full error log. Sorry just pasted one line when I opened the PR.

external/com_github_getml_reflect_cpp/include/rfl/internal/no_duplicate_field_names.hpp:16:26: error: ambiguous overload for 'operator==' (operand types are 'int' and 'const std::tuple_size<std::tuple<rfl::Field<rfl::internal::StringLiteral<12>{
std::array<char, 11>{"op_history"}}, const elixir::utils::RingArray<double>*>, rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 4>{"sum"}}, const double*> > >')
   16 |         if constexpr (_i == num_fields) {
      |                       ~~~^~~~~~~~~~~~~
In file included from ./common/utils/numeric/decimal.h:3,
                 from ./common/utils/decimal.h:1,
                 from ./common/trading/market_data.h:9,
                 from ./common/utils/object_serializer.h:20,
                 from ./common/utils/ring_array.h:9,
                 from ./feature_platform/engine/cc/operators/rolling_sum.h:6:
./common/utils/numeric/decimal128.h:956:6: note: candidate: 'bool elixir::utils::operator==(T, const Decimal128&) [with T = std::tuple_size<std::tuple<rfl::Field<rfl::internal::StringLiteral<12>{std::array<char, 11>{"op_history"}}, const RingArr
ay<double>*>, rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 4>{"sum"}}, const double*> > >]' (reversed)
  956 | bool operator==(T lhs, const Decimal128& rhs) {
      |      ^~~~~~~~
In file included from ./common/utils/numeric/decimal.h:4:
./common/utils/numeric/decimal64.h:335:6: note: candidate: 'bool elixir::utils::operator==(long double, const Decimal64&)' (reversed)
  335 | bool operator==(long double lhs, const Decimal64& rhs);
      |      ^~~~~~~~

An minimal reproducable case, you can add this to your test:

#include "rfl/internal/to_ptr_named_tuple.hpp"
#include <rfl.hpp>

template <typename T>
struct RingArray{};

struct Decimal {
  Decimal(int x) {}
};

template <typename T>
bool operator==(T lhs, const Decimal& rhs) {
  return false;
}

struct A {
  RingArray<double> arr;
  double sum {0};
};

int main() {
  A a;
  auto tv = rfl::internal::to_ptr_named_tuple(a);
}

What I've learned is, we should avoid implict conversion.😂 It's really an error prone.