Update json_value.cpp, fixing up spelling mistakes (boundry to boundary) #1277
Conversation
fix boundry to boundary.
src/lib_json/json_value.cpp
Outdated
| @@ -1401,7 +1401,7 @@ void Value::Comments::set(CommentPlacement slot, String comment) { | |||
| if (!ptr_) { | |||
| ptr_ = std::unique_ptr<Array>(new Array()); | |||
| } | |||
| // check comments array boundry. | |||
| // check comments array boundary. | |||
There was a problem hiding this comment.
Would also need capitalization. Let's just remove the comment.
It's fairly obvious why the if precedes the array element assignment.
This safety check is inconsistently applied. We don't do it in get or has above.
src/lib_json/json_value.cpp
Outdated
| @@ -1401,7 +1401,7 @@ void Value::Comments::set(CommentPlacement slot, String comment) { | |||
| if (!ptr_) { | |||
| ptr_ = std::unique_ptr<Array>(new Array()); | |||
| } | |||
| // check comments array boundry. | |||
|
|
|||
There was a problem hiding this comment.
I don't really want the blank line either.
But while we're manipulating this function let's make the change more meaningful.
void Value::Comments::set(CommentPlacement slot, String comment) {
if (slot >= CommentPlacement::numberOfCommentPlacement)
return;
if (!ptr_)
ptr_ = std::unique_ptr<Array>(new Array());
(*ptr_)[slot] = std::move(comment);
}In other words, if we're not actually going to write to ptr_, we don't need to make one. We don't need to allocate it until we actually use it, and errant calls to set will then have no effect, which is a cleaner recovery I think.
src/lib_json/json_value.cpp
Outdated
| @@ -1398,11 +1398,11 @@ String Value::Comments::get(CommentPlacement slot) const { | |||
| } | |||
|
|
|||
| void Value::Comments::set(CommentPlacement slot, String comment) { | |||
| if (slot >= CommentPlacement::numberOfCommentPlacement) { | |||
There was a problem hiding this comment.
This is not the code I suggested. Can you do it again?
The braces are not needed for the early return.
And the ptr_ assignment needs to happen whether we allocated it in this call or not.
There was a problem hiding this comment.
Yes. If return directly while slot >= CommentPlacement::numberOfCommentPlacement, the CI told that Some checks were not successful.
There was a problem hiding this comment.
I'd be interested in the CI failure.
The problem with this iteration of the code is that the assignment to (*ptr_)[slot] doesn't happen if there wasn't already a ptr_ when the function started. That's not going to work.
But it isn't because of the slot range check. It's because line 1406 should be 2 lines lower, outside the if block.
I'd like to see what happens if you literally take the code I gave earlier, token for token, and paste it in.
Remove redundant comments.
Avoid allocation if the set is going to be rejected anyway. Prototype suggestion from #1277 review thread
* slightly optimize Comments::set Avoid allocation if the set is going to be rejected anyway. Prototype suggestion from #1277 review thread
|
I merged my version of this. Thanks for bringing it up. |
Here is a common spelling mistake that I've found while fixing up spelling mistakes.