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

Make dependencies optional #52

Open
Hendrik-Mueller-Overzone opened this issue Oct 14, 2024 · 4 comments
Open

Make dependencies optional #52

Hendrik-Mueller-Overzone opened this issue Oct 14, 2024 · 4 comments

Comments

@Hendrik-Mueller-Overzone

Hey there,

are there plans to add build flags to disable XML (thus not requiring expat)?
Also are there plans to build in compile time checks, which C++ version is used and only requiring tl-optional and tl-expected, if they are not required?

This would allow users, that are targeting newer C++ versions and embedded targets, to keep the footprint small.

@mristin
Copy link
Contributor

mristin commented Oct 14, 2024

Hi @Hendrik-Mueller-Overzone ,
Our CMake knowledge is limited -- I always thought that tl-optional and tl-expected are required only in older C++ versions. Can you please double-check?

are there plans to add build flags to disable XML (thus not requiring expat)?

This is a good point! I am quite busy at the moment -- would you mind making a pull request? I can review it in a timely manner, but writing & fighting CMake is a bit too much for my plate at the moment.

@Hendrik-Mueller-Overzone
Copy link
Author

Hendrik-Mueller-Overzone commented Oct 15, 2024

Okay, I've investigated a bit and found the following points/issues:

Making JSON and XML Serialization optional is not that hard, I will prepare a pull request.

tl-optional and tl-expected are only used in common.hpp, if they are required, but

  • a) the C++ version is always set to 11 in the CMake file, so they are always required and
  • b) find_library() in CMake will always fail, if the library is not present on the system, even if it is not needed, because it is marked as REQUIRED

My approach would be to only define the C++ version, if it is not already defined and then only include the required libraries, if the defined version is below 17 or 23 respectively. I can also attempt to provide a pull request for that.

While attempting to compile with version 23, I stumbled upon two other issues, which would probably need to be fixed in the code-gen.

  1. std::make_unexpected has not made it to the final version of C++ 23, but is still implemented by tl-expected and used within the codebase. If C++23 is used, the attempted using std::make_unexpected fails. This can be resolved by changing all occurrences of std::make_unexpected to std::unexpected.
  2. In C++23, more stringent checks are introduced regarding the destruction and size calculation of incomplete types. This results in a compile error in verification.hpp line 85, because impl::IVerificator is not completely known at that point. The fix here is to move the implementation of the constructor to verification.cpp

Both changes would need to be made in the code-gen. I can provide the pull request on that repository, but I would feel more confident in a correct result, if you would update this repository with the newly generated code.

@mristin
Copy link
Contributor

mristin commented Oct 15, 2024

Hi @Hendrik-Mueller-Overzone ,
Thanks! Let's first fix the JSON/XML requirements, and then focus on the expected/optional. For that second point (expected/optional), I think it would be easiest if we set up a quick call to sync up. Would you mind sending me a private message through email? (See the commit history for my email address.)

@mristin
Copy link
Contributor

mristin commented Oct 22, 2024

@Hendrik-Mueller-Overzone just a kind reminder -- please let me know if you need any help from my side.

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

2 participants