Skip to content

Do not reference SecureAllocator when not JSONCPP_USING_SECURE_MEMORY #872

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

Closed
wants to merge 1 commit into from

Conversation

pps83
Copy link

@pps83 pps83 commented Jan 19, 2019

Amalgamated version of jsoncpp does not include allocator.h and as a result doesn't have definition of SecureAllocator and results in compilation error when referencing SecureAllocator in config.h

this issue is mentioned in #871

@pps83 pps83 force-pushed the SecureAllocator-fix branch 2 times, most recently from 6622c9d to 46acbc8 Compare January 19, 2019 18:50
@pps83 pps83 changed the title Do not reference SecureAllocator when JSONCPP_USING_SECURE_MEMORY==0 Do not reference SecureAllocator when not JSONCPP_USING_SECURE_MEMORY Jan 19, 2019
@hjmjohnson
Copy link
Contributor

LGTM, but I'm going to let other have a chance to weigh in.

@pps83
Copy link
Author

pps83 commented Jan 20, 2019

if you use amalgamated version then it would have this code:

using Allocator = typename std::conditional<JSONCPP_USING_SECURE_MEMORY,
                                            SecureAllocator<T>,
                                            std::allocator<T>>::type;

but there would be no SecureAllocator defined anywhere. In other words, it would totally rely on compiler ignoring that template parameter to std::conditional. Proper fix would perhaps be to add allocator.h to amalgamated version, but in any case this change is good imo and there was no point in the first place to go through std::conditional.

@BillyDonahue
Copy link
Contributor

I think we should add allocator.h to amalgamated.

I would much rather use in-language C++ tools like std::conditional whenever we can.
This ensures that everything is seen by the C++ compiler and must be valid code.
If we use #if to shut code off completely, we won't notice if it fails to compile somewhere.

Pavel thanks for the hint about amalgamated being the key issue here.
I have no idea why allocator.h wouldn't be included in the amalgamated header. It really should be.
I'm gladd we can now see from this episode that we should be doing CI tests on the amalgamated form of the library.

Copy link
Contributor

@BillyDonahue BillyDonahue left a comment

Choose a reason for hiding this comment

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

I don't agree with this direction.
We should just add allocator.h to the amalgamated distro.

@@ -173,11 +173,11 @@ typedef UInt64 LargestUInt;
#define JSON_HAS_INT64
#endif // if defined(JSON_NO_INT64)

template <typename T>
using Allocator = typename std::conditional<JSONCPP_USING_SECURE_MEMORY,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still have a Json::Allocator alias template.
If there's fancy conditional or #if tap dancing to calculate the Allocator alias, let's have it contained to the Allocator declaration and not infect things like Json::String. The definition of Json::String should just use Json::Allocator unconditionally.

BillyDonahue added a commit to BillyDonahue/jsoncpp that referenced this pull request Jan 20, 2019
…d header.

I don't know why we didn't include this before.
It seems to work fine.
@pps83 pps83 force-pushed the SecureAllocator-fix branch from 46acbc8 to 2874474 Compare January 20, 2019 05:29
Amalgamated version of jsoncpp does not include allocator.h and as a result doesn't have definition of SecureAllocator and results in compilation error when referencing SecureAllocator in config.h
@pps83 pps83 force-pushed the SecureAllocator-fix branch from 2874474 to 0a01f3d Compare January 20, 2019 05:31
BillyDonahue added a commit that referenced this pull request Jan 21, 2019
I don't know why we didn't include this before.
It seems to work fine.
@hjmjohnson
Copy link
Contributor

@BillyDonahue Are you happy with this improvement? Can it be merged, or should it be abandoned?

@hjmjohnson
Copy link
Contributor

Adding json/allocator.h in the amalgamated header seems to have resolved this issue in an alterantive way.

Closing this issue. Please re-open if other issues exist.

@hjmjohnson hjmjohnson closed this Apr 28, 2019
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.

3 participants