Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relative includes #30

Closed
vaijns opened this issue Dec 15, 2023 · 2 comments
Closed

relative includes #30

vaijns opened this issue Dec 15, 2023 · 2 comments

Comments

@vaijns
Copy link

vaijns commented Dec 15, 2023

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

  • from_named_tuple.hpp
  • fields.hpp
  • Validator.hpp
  • Timestamp.hpp
  • PatternValidator.hpp
  • Literal.hpp

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
Copy link
Contributor

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

@ChemiCalChems
Copy link
Contributor

ChemiCalChems commented Dec 22, 2023

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.

liuzicheng1987 added a commit that referenced this issue Dec 28, 2023
Change project includes to relative (related to #30)
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

No branches or pull requests

3 participants