getml / reflect-cpp

A C++20 library for fast serialization, deserialization and validation using reflection. Supports JSON, BSON, CBOR, flexbuffers, msgpack, TOML, XML, YAML / msgpack.org[C++20]
https://getml.github.io/reflect-cpp/
MIT License
821 stars 65 forks source link

get_field_name_str_view() doesn't work with clang on windows #56

Closed grandseiken closed 5 months ago

grandseiken commented 5 months ago

The following snippet fails to compile under clang-cl version 17.0.6 and Visual Studio 17.8.5 with /std:c++latest.

#include <rfl.hpp>

struct test {
  bool x;
  bool y;
};

void f() {
  for (const auto& f : rfl::fields<test>()) {}
}

The same snippet compiles successfully when building with the microsoft compiler, or if I remove one of the fields in the struct.

The compilation error seems to be some sort of static_assert about duplicate string literals:

In file included from tools/test.cc:1:
In file included from external/_main~_repo_rules~reflect_cpp/include\rfl.hpp:12:
In file included from external/_main~_repo_rules~reflect_cpp/include/rfl/Attribute.hpp:10:
external/_main~_repo_rules~reflect_cpp/include/rfl/Literal.hpp(295,17): error: static assertion failed due to requirement '!has_duplicates()': Duplicate strings are not allowed in a Literal.
  295 |   static_assert(!has_duplicates(),
      |                 ^~~~~~~~~~~~~~~~~
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/get_field_names.hpp(97,15): note: in instantiation of template class 'rfl::Literal<internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}, internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}>' requested here
   97 |   return rfl::Literal<_names1..., _names2...>::template from_value<0>();
      |               ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/get_field_names.hpp(105,12): note: in instantiation of function template specialization 'rfl::internal::concat_two_literals<internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}, internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}>' requested here
  105 |     return concat_two_literals(_head, concat_literals(_tail...));
      |            ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/get_field_names.hpp(127,14): note: in instantiation of function template specialization 'rfl::internal::concat_literals<rfl::Literal<internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}>, rfl::Literal<internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}>>' requested here
  127 |       return concat_literals(
      |              ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/get_field_names.hpp(138,12): note: in instantiation of function template specialization 'rfl::internal::get_field_names()::(anonymous class)::operator()<0ULL, 1ULL>' requested here
  138 |     return get(std::make_index_sequence<num_fields<T>>());
      |            ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../field_names_t.hpp(14,24): note: in instantiation of function template specialization 'rfl::internal::get_field_names<test>' requested here
   14 |     decltype(internal::get_field_names<std::remove_cvref_t<T>>)>::type;
      |                        ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/to_ptr_named_tuple.hpp(68,29): note: in instantiation of template type alias 'field_names_t' requested here
   68 |     using FieldNames = rfl::field_names_t<T>;
      |                             ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/ptr_named_tuple_t.hpp(16,42): note: in instantiation of function template specialization 'rfl::internal::to_ptr_named_tuple<test>' requested here
   16 |     typename std::invoke_result<decltype(to_ptr_named_tuple<T>), T>::type;
      |                                          ^
external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../named_tuple_t.hpp(39,1): note: in instantiation of template type alias 'ptr_named_tuple_t' requested here
   39 | using named_tuple_t =
      | ^
external/_main~_repo_rules~reflect_cpp/include/rfl/fields.hpp(12,36): note: in instantiation of template type alias 'named_tuple_t' requested here
   12 |   return internal::get_meta_fields<named_tuple_t<T>>();
      |                                    ^
tools/test.cc(9,29): note: in instantiation of function template specialization 'rfl::fields<test>' requested here
    9 |   for (const auto& f : rfl::fields<test>()) {}
      |
liuzicheng1987 commented 5 months ago

Hi, @grandseiken, thanks for opening the issue.

I can't reproduce the issue using Ubuntu clang version 17.0.6. When I try to compile your code snippet, it compiles just fine.

The clue is in this line, though:

external/_main~_repo_rules~reflect_cpp/include/rfl/internal/../internal/get_field_names.hpp(97,15): note: in instantiation of template class 'rfl::Literal<internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}, internal::StringLiteral<22>{{{103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ...}}}>' requested here

The ASCII strings 103, 101, 116, 95, 102, 105, 101, 108, 100, 95, ... translate to getfield...

I am fairly certain that for some reason get_field_names_str_view is not doing what it is supposed to be doing. The function can be found in here:

https://github.com/getml/reflect-cpp/blob/main/include/rfl/internal/get_field_names.hpp

The question is why...we have rigorously tested this on clang and MSVC and I cannot reproduce the error. What is your OS? Can you think of anything that might be different on your machine?

liuzicheng1987 commented 5 months ago

I am also a bit confused...you said it compiles with "the microsoft compiler", but not Visual Studio 17.8.5? Isn't Visual Studio 17.8.5 by Microsoft? Could you clarify that?

grandseiken commented 5 months ago

clang-cl is a compiler driver which allows building with clang on Windows while using the microsoft standard library. https://clang.llvm.org/docs/UsersManual.html#clang-cl it's (afaik) the easiest way to build with clang on windows.

Have you tried building with clang on windows at all?

I tried fixing it myself, but it seems the method you're using to get field names might just not work under clang-cl. I'm not sure if I'm invoking the function in exactly the same way as the library does, but the following program only prints get_field_name_str_view, i.e. it seems the template arguments are not included in the function_name() of std::source_location.

#include <iostream>
#include <source_location>
#include <string_view>
struct test {};

template <class T, auto ptr>
consteval auto get_field_name_str_view() {
  const auto func_name =
      std::string_view{std::source_location::current().function_name()};
  return func_name;
}

const int y = 0;
int main(int argc, const char** argv) {
  constexpr const int* x = &y;
  std::cout << get_field_name_str_view<test, x>() << '\n';
  return 0;
}
grandseiken commented 5 months ago

I just checked and I get the same result building with the standard windows clang++.exe 17.0.6 downloaded from the LLVM releases page, so I think the problem is not exclusive to clang-cl, rather clang on windows generally.

grandseiken commented 5 months ago

It seems that __PRETTY_FUNCTION__ does work as expected in the above test, i.e. I get auto get_field_name_str_view() [T = test, ptr = &y] as (presumably) expected.

I can put up a PR to use that instead when __clang__ and _MSC_VER are both defined, if you'd be willing to accept it? I know the docs say "Standard C++ only, no compiler-specific macros", but as far as I see the implementation of get_field_name_str_view and similar functions is already not really standard C++ by depending on the exact format of implementation-defined strings.

liuzicheng1987 commented 5 months ago

Yes, I would accept the PR. Thank you so much for flagging this and for coming up with a fix.