Allow non-deprecated removeMember/removeIndex with null return pointer #689
Conversation
Sometimes we just want to remove something we don't need anymore. Having
to supply a return buffer for the removeMember/removeIndex function to
return something we don't care about is a nuisance. There are remove-
Member/Index functions that don't need a return pointer but they are
deprecated. This commit allows the non-deprecated versions to be called
with a null pointer as the last argument to simplify things for callers
who don't need to get something back.
For example:
Json::Value j;
j.removeMember("name"); // nice but deprecated
Json::Value r; j.removeMember("name",&r); // possible but annoying
j.removeMember("name",nullptr); // possible with this commit
|
I feel it would be better to just un-deprecate the unary member functions if they're useful. |
|
@BillyDonahue I agree :) but I'm afraid they were deprecated for a reason. OTOH, the functions that receive the |
|
Well, there are lots of invalid pointers in the memory space, and we can't handle them all gracefully, so I don't like going out of my way to Nerf just the invalid nullptr argument. As for why it's a pointer instead of a reference, I suspect it was just a style choice to make the mutability more obvious at the call sites (matching, e.g. Google style). I wonder about the deprecation. I can't think of a reason for it so maybe it's reversible. That's a better API change than adding a new function IMO because it's not really an API change at all! |
|
I'm not positive, but I think those were deprecated because of their misleading return values. They returned I guess we could undeprecate, but only to return nothing. I gues returning |
|
Can we simply change the functions to return void instead of a value? This could break client code. If it's ok by you, I'll submit it as a new pull request. |
|
If I were you, I would simply re-use this branch and this pull-request, after rebasing. |
|
I'm a bit reluctant to force push a branch that was pushed already. Also, should have used a branch in the first place, instead of editing on master. Submitted a new pull request: #693 |
|
@wolframroesler, yes, if you use feature-branches, then you'll have less worry about a force-push. Anyway, I'll merge the new PR after the minor bug is fixed and the tests pass. Thanks. |
|
Too good I decided not to follow your advice about the force push ... :-) |
* Un-deprecate removeMember overloads, return void Sometimes we just want to remove something we don't need anymore. Having to supply a return buffer for the removeMember function to return something we don't care about is a nuisance. There are removeMember overloads that don't need a return buffer but they are deprecated. This commit un-deprecates these overloads and modifies them to return nothing (void) instead of the object that was removed. Further discussion: #689 WARNING: Changes the return type of the formerly deprecated removeMember overloads from Value to void. May break existing client code. * Minor stylistic fixes Don't explicitly return a void value from a void function. Also, convert size_t to unsigned in the CZString ctor to avoid a compiler warning.
Sometimes we just want to remove something we don't need anymore. Having to supply a return buffer for the removeMember/removeIndex function to return something we don't care about is a nuisance. There are removeMember/removeIndex functions that don't need a return pointer but they are deprecated. This commit allows the non-deprecated versions to be called with a null pointer as the last argument to simplify things for callers who don't need to get something back.
For example:
Note: Implementation only (albeit very trivial), no test coverage.