Skip to content

Jsoncpp string #442

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

Merged
merged 3 commits into from
Mar 20, 2016
Merged

Jsoncpp string #442

merged 3 commits into from
Mar 20, 2016

Conversation

cdunn2001
Copy link
Contributor

After std::string -> JSON_CPP, these are the actual changes. For discussion with @dawesc of ETFlab.

@dawesc
Copy link
Contributor

dawesc commented Mar 18, 2016

Hi Chris, many thanks, I think it's looking good!

? duplicateStringValue(other.cstr_, other.storage_.length_)
: other.cstr_) {
Value::CZString::CZString(const CZString& other) {
cstr_ = (other.storage_.policy_ != noDuplication && other.cstr_ != 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about this change. In general, it's slightly faster to a initialization list, but not for built-in types like pointers. So I don't care. Just curious.

@cdunn2001
Copy link
Contributor Author

Almost ready to merge, but I think we need something in cmake to allow this feature to be enabled.

If it is built one way, but users supply a different setting for the macro, that would be a problem. It might be better for cmake to substitute something in some header, and to let everything rely on that.

#define JSONCPP_USE_SECURE_ALLOC %%TO_BE_SUBSTITUTED_BY_CMAKE%%

@cdunn2001 cdunn2001 force-pushed the JSONCPP_STRING branch 2 times, most recently from 7f7e013 to 44159ac Compare March 19, 2016 20:49
@cdunn2001
Copy link
Contributor Author

Any objections?

@cdunn2001
Copy link
Contributor Author

An error from TravisCI:

[ 22%] Building CXX object src/lib_json/CMakeFiles/jsoncpp_lib.dir/json_value.cpp.o
/home/travis/build/open-source-parsers/jsoncpp/src/lib_json/json_value.cpp: In function ‘void Json::releasePrefixedStringValue(char*)’:
/home/travis/build/open-source-parsers/jsoncpp/src/lib_json/json_value.cpp:146:10: error: conversion to ‘unsigned int’ from ‘long unsigned int’ may alter its value [-Werror=conversion]
   length += sizeof(unsigned) + 1;
          ^
cc1plus: some warnings being treated as errors

@cdunn2001
Copy link
Contributor Author

Another one:

/home/travis/build/open-source-parsers/jsoncpp/src/lib_json/json_value.cpp: In function ‘void Json::releasePrefixedStringValue(char*)’:
/home/travis/build/open-source-parsers/jsoncpp/src/lib_json/json_value.cpp:145:59: error: cannot convert ‘size_t* {aka long unsigned int*}’ to ‘unsigned int*’ for argument ‘3’ to ‘void Json::decodePrefixedString(bool, const char*, unsigned int*, const char**)’
   decodePrefixedString(true, value, &length, &valueDecoded);

Add allocator.h to amalgamated header
Test JSONCPP_USE_SECURE_MEMORY in Travis
cdunn2001 added a commit that referenced this pull request Mar 20, 2016
Secure allocator available for wiping memory.

Resolves #437.
Resolves #152.
@cdunn2001 cdunn2001 merged commit c018d9f into master Mar 20, 2016
@cdunn2001 cdunn2001 deleted the JSONCPP_STRING branch March 20, 2016 00:31
@dawesc
Copy link
Contributor

dawesc commented Mar 20, 2016

Hi Chris, thanks for merging this, you had asked some questions e.g.:

I was wondering about this change. In general, it's slightly faster to a initialization list, but not for built-in types like pointers. So I don't care. Just curious.

http://stackoverflow.com/questions/13214241/c-constructors-vs-initialization-lists-speed-comparison

However they've disappeared, would you like me to answer them in this pull request or are you happy? (p.s. the above was because i had trouble to initialize the list during one of the iterations in the initialization list due to needing an ifdef in the initialization list and the code was becoming unreadable so i sacrificed cpu for maintainability however apologies it should have gone back again!)

@cdunn2001
Copy link
Contributor Author

I think we're fine. Hopefully, no-one will mind the macros too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants