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

Change signature of newCharReader to return uniqu_ptr #1422

Open
AssafPassalCG opened this issue Jul 23, 2022 · 5 comments
Open

Change signature of newCharReader to return uniqu_ptr #1422

AssafPassalCG opened this issue Jul 23, 2022 · 5 comments

Comments

@AssafPassalCG
Copy link

Description
The current implementation of Json::CharReaderBuilder.newCharReader(), allocates memory, while there is no one to release it,
this issue causes a leak because when we upgraded to the new version, we didn't understand we should wrap the pointer with std::unique_ptr

Solution
Make the function return std::unique_ptr

Context
I opened that PR: #1420, and that closed because it is a breaking change. While I see the point of that, I actually think that if it will break someone's code, there is a good chance that it will save him from a memory leak.

@BillyDonahue
Copy link
Contributor

BillyDonahue commented Jul 23, 2022

We can't change the signature of this old and fundamental function.

We have a lot of user code written for that raw pointer return type, and changing it would require a change to nearly all programs that use the jsoncpp library. It's not IMO a problem requiring such a drastic change. C++ programmers are assumed to take care with raw pointers and avoid memory leaks, and ideally install leak detectors on their CI builds.

What we should do, however, is provide a similar but different function that returns a std::unique_ptr<CharReader>, and encourage transition to that one. Maybe makeCharReader? newCharReader can be discouraged and deprecated down the road but we do not need to remove it.

@AssafPassalCG
Copy link
Author

I will create the PR, soon I hope

@AssafPassalCG
Copy link
Author

Not sure how to mark the deprecated function

@AssafPassalCG
Copy link
Author

#1424

@AssafPassalCG
Copy link
Author

I finished the PR changes you requested, and most of the changes happened because I used reformat.sh script from the repo, maybe it a good idea to put some note that you shouldn't commit those changes

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

No branches or pull requests

2 participants