search

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
901 stars 76 forks source link

General constructive criticism #10

Closed Mr-S-Mirzoev closed 7 months ago

Mr-S-Mirzoev commented 10 months ago

Hey there! I was happy to learn about a library for reflection in C++. On C++ Developers LinkedIn group, you asked for constructive criticism, so here are a couple of bits of it:)

First of all, I think that for a shared library, one could make use of link-time optimisation [see: GCC Wiki on LTO]. For that, it's better to add yyjson as a dependency and then link it to the library.

Also, it is a slight disturbance and may seem picky, but why lowercase and uppercase starts in the names of the included files are mixed? It may make sense in languages like GoLang, but in C++, it's just confusing, IMHO.

I'll dive deeper in the following weeks to give more criticism on the code. But overall, it looks like an incredible tool!

liuzicheng1987 commented 10 months ago

Thanks for your input!

To your points:

  1. If you want to link YYJSON yourself, nothing is stopping you. In that case, reflect-cpp is header-only. I think we could make that a bit clearer in the installation instructions, but principally speaking I think the way currently do it is to ship YYJSON to make it easier for users, but we are not stopping anybody from the linking their own versions. So I think that is the appropriate way to go.

  2. There is a clear difference: Things that start uppercase are classes, whereas things that start with lowercase are either functions or auxiliary templates. This is, because we use Google Style (https://google.github.io/styleguide/cppguide.html) where this is handled similarly.

I will leave the issue open, if you have more feedback.