Pass parameters by value to StreamHandlerError
#40311
Conversation
| #include <memory> | ||
| #include <string> |
There was a problem hiding this comment.
Don't break if this header file is imported before/without these imports.
| result = codec->EncodeErrorEnvelope(error->error_code, | ||
| error->error_message, | ||
| error->error_details); | ||
| error->error_details.get()); |
There was a problem hiding this comment.
This method points to an abstract method that we implement in the standard and json codecs, both of which copy the value pointed to by the parameter.
| std::string code = "Code"; | ||
| std::string msg = "Message"; | ||
| std::unique_ptr<EncodableValue> details = std::make_unique<EncodableValue>("Details"); |
There was a problem hiding this comment.
Declare these inside a nested scope so they are out of scope before our asserts, to test pass-by-value.
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
LGTM but please also get an approval from Stuart
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with one added test.
flutter/flutter#101682 (comment) suggests that existing projects only ever pass
nullptrto this parameter, so changing to aunique_ptrwill not break them.
Fingers crossed that this is still true. (If there's significant unexpected fallout we may have to revert and figure out another plan.)
| std::unique_ptr<EncodableValue> details = | ||
| std::make_unique<EncodableValue>("Details"); | ||
| error = | ||
| std::make_unique<StreamHandlerError<>>(code, msg, std::move(details)); |
There was a problem hiding this comment.
Let's add a second test that does a (code, msg, nullptr) so that we're absolutely certain that we aren't breaking that existing-in-the-wild pattern.
yaakovschectman
force-pushed
the
stream_handler
branch
from
ae09744 to
2214dee
Compare
4 participants
StreamHandlerErrorpreviously took references to its parameters, which means if the error persists longer than its calling scope, it may point to nonexistent values and crash the program. Change the constructor to take the string parameters as value rather than reference, and take its details as aunique_ptrrather than a raw pointer. flutter/flutter#101682 (comment) suggests that existing projects only ever passnullptrto this parameter, so changing to aunique_ptrwill not break them.The
detailsfield is potentially passed toEncodeErrorEnvelopeInternal. Our implementations of this abstract method in the standard and json codecs copy the value pointed to by the parameter, so passing the raw pointer behind the unique pointer should not lead to dangling pointers.flutter/flutter#101682
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.