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

A generic basic_claim to allow support for other libraries #71

Merged
merged 61 commits into from
Jun 26, 2020

Conversation

prince-chrismc
Copy link
Collaborator

In full disclosure, generic template meta-programming containers has been a learning curve

Take a look, certainly can use feedback on this. Maybe it's not something that fits this project 🤷‍♂️

Currently I am using 3 JSON libraries, and have 3 base64 implementations so having the ability to switch out the the underlying library would be a huge asset. Others have asked for this so I went ahead with my best attempt.

Open question, should there be official nlohmann/json support?

I need to address the CMake I would like to add a few options to exclude picojson.h and base.h since it will be possible to no use those.

Another point to look at, perhaps user feedback, the number of template parameters. Its very possible to have a single one traits which has all the type definitions required.

I this gets merged it would be nice to have a pre-release and post it online for feedback 🤞 to get this over the hump.

One goal was to not break the API, there should be zero changes that affect any consumer. there's only one change in the existing test code

@Thalhammer
Copy link
Owner

I'll take a look at it tomorrow, but I certainly like the idea of having base64/json libraries exchangeable.
The base64 is not really visible on any api so thats probably less of a priority, but the json data hits user code, so it would be a huge benefit to support a library they already know.
When I initially wrote this library the goal was to support a personal project and since I use picojson everywhere and nothing else It was never a design consideration.

running clang-diagnostic-*,clang-analyzer-*,-*,bugprone*,modernize*,performance*,-modernize-pass-by-value,-modernize-use-auto,-modernize-use-using
- google-readability-namespace-comments
- hicpp-uppercase-literal-suffix
- llvm-include-order
- llvm-header-guard
- readability-const-return-type
- readability-else-after-return
- hicpp-special-member-functions
- google-readability-casting
moved to two more generic type trait validation
tests/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines +420 to +421
rr.insert(0, signature_length/2 - rr.size(), '\0');
rs.insert(0, signature_length/2 - rs.size(), '\0');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a second set of eyes

@prince-chrismc prince-chrismc marked this pull request as ready for review June 11, 2020 01:25
@prince-chrismc
Copy link
Collaborator Author

The last open question is do we official support nlohmann/json?

Short of that I think I've checked all the boxes. When you get the chance, love for a code review 😃


const auto string = nholmann_claim(std::string("string"));
const auto array = nholmann_claim(std::set<std::string>{"string", "string"});
const auto integer = nholmann_claim(159816816);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nlohmann not nholmann

@Thalhammer
Copy link
Owner

Thalhammer commented Jun 26, 2020

I am sorry it took me this long to take a proper look at the amazing work you did here.
I do like what I see, however I have a couple of minor things I would do differently.

  • You seem to import a couple of C++14/C++17 features.
    There are feature test macros which are defined by the compiler if a given feature is provided by the stdlib (See here).
    While not a big deal, it might be a good idea to use the "official" one if supported.
  • I honestly dont like macros so I might be a bit biased, but I don't see a reason not to move the types into the json trait and passing only the traits around, instead of using a macro to add them. Is there a reason why you did it this way ?

None of the above are a big deal/show stopper, though.

EDIT: I am currently converting the code to keep all types in the traits instead of single params.
EDIT2: Done moving the types into json_traits, I'll postpone using stdlib if possible, as this would require changing the travis task to build against more target (C++11, C++14, C++17 & maybe also clang)

@Thalhammer Thalhammer merged commit c2d4c08 into Thalhammer:master Jun 26, 2020
@prince-chrismc
Copy link
Collaborator Author

That's absolutely all right, one thing i learnt is every one is too busy!

You seem to import a couple of C++14/C++17 features.
There are feature test macros which are defined by the compiler if a given feature is provided by the stdlib (See here).
While not a big deal, it might be a good idea to use the "official" one if supported.

Totally in agreement.

The only feature I implementedcopied myself was std::is_detected which is not even apart of the standard, __cpp_lib_experimental_detect, I'll add it to my agenda for the next rainy weekend 😄

If there are any other's let me know.

I honestly dont like macros so I might be a bit biased, but I don't see a reason not to move the types into the json trait and passing only the traits around, instead of using a macro to add them. Is there a reason why you did it this way ?

I was following the nlohmann/json implementation. I had been considering it myself. It was in my opening comment!

@Thalhammer
Copy link
Owner

If there are any other's let me know.

make_void was the one that jumped my eye (which is c++17), but I honestly dont know the standards after C++14 well enough to judge what else might get replaced.

It was in my opening comment!

Sorry missed that :)

@prince-chrismc
Copy link
Collaborator Author

I think all the possible features are int eh same block so it should not be too hard to check them. __cpp_lib_void_t might cover make_void

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

Successfully merging this pull request may close these issues.

None yet

3 participants