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

Fix memory leak during parser callback #1001

Merged
merged 2 commits into from
Mar 9, 2018
Merged

Fix memory leak during parser callback #1001

merged 2 commits into from
Mar 9, 2018

Conversation

nlohmann
Copy link
Owner

@nlohmann nlohmann commented Mar 9, 2018

Fixes a memory leak in the case when the parser callback function discards values. The reason was a forgotten destroy call.

Thanks to @RalfBielig for reporting and proposing a fix.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling aa8fc2a on leak into cf60e18 on develop.

@nlohmann nlohmann self-assigned this Mar 9, 2018
@nlohmann nlohmann added this to the Release 3.1.2 milestone Mar 9, 2018
@nlohmann nlohmann merged commit e737de8 into develop Mar 9, 2018
@nlohmann nlohmann deleted the leak branch March 9, 2018 20:23
nlohmann added a commit that referenced this pull request Mar 9, 2018
@@ -953,7 +953,7 @@ class basic_json
/// constructor for rvalue strings
json_value(string_t&& value)
{
string = create<string_t>(std::move(value));
string = create<string_t>(std::forward < string_t&& > (value));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::move was correct here (and in all the other places in this PR) since value is not a forwarding reference, but an rvalue reference.

The behaviour is the same in this case, but is quite confusing.

@nlohmann
Copy link
Owner Author

Thanks, I’ll change that. I was totally confused that the memory leak went undetected for so long.

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.

3 participants