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

relative includes #30

Closed vaijns closed 8 months ago

vaijns commented 8 months ago

Most includes seem to use an absolute path (starting with "rfl/...") with some exceptions I found:

which have some relative paths defined as well (might wanna unify that).

However I think this makes it a bit harder to include the library just for some quick testing as you have to specify an include directory (which you'd probably do for production anyways though ofc).

For me I like to try out new (header-only) libraries by just git init and git submodule add https://github.com/getml/reflect-cpp libs/reflect-cpp and using it in a simple main.cpp with #include "libs/reflect-cpp/include/rfl.hpp", running clang/gcc for compilation directly. This doesn't work with these absolute paths though as:

rfl.hpp includes rfl/AllOf.hpp (fine so far) which then includes rfl/Result.hpp which is at the same level, as we are already in the rfl directory. So the include trying to go into rfl a level deeper doesn't work unless I add libs/reflect-cpp/include/ as an include directory (compiling with g++ -std=c++20 -I ./libs/reflect-cpp/include/ main.cpp), so it can always go "back" into the include directories root rfl-directory while with relative paths it should just work right out of the box without that.

also the described way to

Simply copy the contents of the folder include into your source repository OR add it to your include path.

doesn't quite work due to this (unless the 'OR' is meant to be an 'and' instead).

Not a big deal of course and relative paths have their cons while absolute paths have their pros, so you might wanna consider changing it but it's fine if you don't.

btw., that's a cool and useful library, definitely gonna consider using it for future projects. :)

liuzicheng1987 commented 8 months ago

Yeah, that makes sense. I think we should be consistent.

ChemiCalChems commented 8 months ago

The more time passes, the more I think we should've gone the opposite way on this issue, as @vaijns originally suggested.

The rationale is we shouldn't expect users to have to set any extra include directories when using our header-only library as a drop-in. The headers should include each other in a relative manner so no matter where our library is, no matter what the user's build system looks like, if he manages to include one of our files, then it can find others because they are all packaged together anyway.

I've been looking around some header-only libraries and this seems to be done quite often, Hanickadot's CTRE is a good example of this practice.

I hereby request this issue be reopen, that all includes be changed to relative paths, and that we add somewhere in the README that developers should strictly adhere to this for the stability of the library. I'm currently looking for a clang-format / clang-tidy flag to could help us with this, maybe now that we have Github Actions a custom action could be written, I'll look into that too.

This will become even more of a problem the moment we start adding more dependencies. We should solve this now.

In the meantime, I'll get a PR going.