Use references for C++ MethodResult and EventSink #20651
Conversation
The reponse APIs for method channels and event channels used pointers for optional parameters; this kept the API surface simple, but meant that they couldn't take rvalues. As a result, returning success values or error details often took an extra line, declaring a variable for the result just to have something to pass the address of. This converts them to using references, with function overloading to allow for optional parameters, so that values can be inlined. For now the pointer versions are still present, so that conversion can be done before it becomes a breaking change; they will be removed soon. Part of flutter/flutter#63975
| void Success() { SuccessInternal(nullptr); } | ||
|
|
||
| // Consumes an error event. | ||
| // DEPRECATED. Use the reference version below. This will be removed in the |
There was a problem hiding this comment.
Do we do the C++14 deprecated annotation thing? Probably not, since warnings-are-errors, but it seems useful.
i.e.:
[[deprecated("Use the reference version instead.")]]
void Error(const std::string& error_code,
const std::string& error_message,
const T* error_details) {
ErrorInternal(error_code, error_message, error_details);
}There was a problem hiding this comment.
Probably not, since warnings-are-errors
Exactly. I tried that the last time I did one of these and realized it would just make it an immediate breaking change, which is unfortunate.
In practice, since Windows is still subject to change without warning, it's not a big deal (I'm doing it in stages so I can not break FDE and google3 without having a chance to pre-fix them), but long term it would be nice if we could use them to do soft breaking changes. It's a potential argument for relaxing the template to not treat warnings as errors.
| #include <functional> | ||
| #include <string> | ||
|
|
||
| #include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_result_functions.h" |
There was a problem hiding this comment.
I thought that the header corresponding to the cc file always came first... Am I just wrong? Is it supposed to come after the system headers like this? Or is that only in the non-test files?
There was a problem hiding this comment.
It should; the style guide is explicit about this. But for reasons that aren't clear to me, clang-format in the engine repo keeps moving the header down in test files.
I keep meaning to look into it, then forgetting. I'll file a bug. Maybe it only understands _test, and not _unittests? But I swear it used to leave it alone.
3 participants
Description
The reponse APIs for method channels and event channels used pointers
for optional parameters; this kept the API surface simple, but meant
that they couldn't take rvalues. As a result, returning success values
or error details often took an extra line, declaring a variable for the
result just to have something to pass the address of.
This converts them to using references, with function overloading to
allow for optional parameters, so that values can be inlined.
For now the pointer versions are still present, so that conversion can
be done before it becomes a breaking change; they will be removed soon.
Related Issues
Part of flutter/flutter#63975
Tests
I added the following tests: Updated existing tests to use reference versions.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.