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

AddMember() may be incorrectly used. #38

Closed
miloyip opened this issue Jul 1, 2014 · 0 comments
Closed

AddMember() may be incorrectly used. #38

miloyip opened this issue Jul 1, 2014 · 0 comments

Comments

@miloyip
Copy link
Collaborator

miloyip commented Jul 1, 2014

In upstream issue 66,

user misuse the AddMember() API:

doc.AddMember(lvl.c_str(), score, doc.GetAllocator());

lvl.c_str() is a temp string, it needs to be duplicated, otherwise when the function returns the pointer become invalid.

For this situation, the user should call

rapidjson::Value name(lvl.c_str(), doc.GetAllocator()); // copy the string
rapidjson::Value value(score);
doc.AddMember(name, value, doc.GetAllocator());

But it may be confusing for user.

All overloads of AddMember(const char* name,... ) assume the name being literal string, no need for making copies. But it may be misused like this.

Any suggestion to improve the API for DOM manipulation?

miloyip added a commit that referenced this issue Jul 3, 2014
4 overloads of `AddMember()` is reduced to 3.
Use copy string name by default to prevent user accidentally do the
wrong thing.
User may still use `AddMember(Value("Hello"), v)` for constant string
name.
Resolves #38
@miloyip miloyip closed this as completed Jul 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant