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.

Sign up for GitHub

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

Conform to clang_tidy in client_wrapper headers. #46058

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conform to clang_tidy in client_wrapper headers. #46058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a052c97
Conform to clang_tidy in client_wrapper headers.
matanlurey Sep 19, 2023
a7c19fb
Merge branch 'main' into engine-clientWrapperClangTidy
matanlurey Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions shell/platform/common/client_wrapper/include/flutter/basic_message_channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <iostream>
#include <string>
#include <utility>

#include "binary_messenger.h"
#include "message_codec.h"
Expand Down Expand Up @@ -58,7 +59,8 @@ class BasicMessageChannel {
void Send(const T& message, BinaryReply reply) {
std::unique_ptr<std::vector<uint8_t>> raw_message =
codec_->EncodeMessage(message);
messenger_->Send(name_, raw_message->data(), raw_message->size(), reply);
messenger_->Send(name_, raw_message->data(), raw_message->size(),
std::move(reply));
}

// Registers a handler that should be called any time a message is
Expand All @@ -77,7 +79,7 @@ class BasicMessageChannel {
BinaryMessageHandler binary_handler = [handler, codec, channel_name](
const uint8_t* binary_message,
const size_t binary_message_size,
BinaryReply binary_reply) {
const BinaryReply& binary_reply) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is BinaryReply not movable? It looks like it should be:

engine/shell/platform/common/client_wrapper/include/flutter/binary_messenger.h

Lines 16 to 17 in 28f14e6

typedef std::function<void(const uint8_t* reply, size_t reply_size)>
BinaryReply;

Copy link
Contributor Author

@matanlurey matanlurey Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A const reference is not copied, it essentially accomplishes the same thing

(To be clear, this is the result of clang_tidy --fix)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my message from: https://discord.com/channels/608014603317936148/608021010377080866/1153752411614220310

The reference argument doesn't copy, but doesn't assigning the reference argument to a non-reference instance field result in a copy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more potential argument for supporting copy elision here and sprinkling in some moves, but setting handlers isn't a common operation, so absent evidence that we need it we should probably just change to the reference. I don't think I did this intentionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BinaryReply and BinaryMessageHandler are std::functions.

engine/shell/platform/common/client_wrapper/include/flutter/binary_messenger.h

Lines 13 to 24 in 10c4803

// A binary message reply callback.
//
// Used for submitting a binary reply back to a Flutter message sender.
typedef std::function<void(const uint8_t* reply, size_t reply_size)>
BinaryReply;
// A message handler callback.
//
// Used for receiving messages from Flutter and providing an asynchronous reply.
typedef std::function<
void(const uint8_t* message, size_t message_size, BinaryReply reply)>
BinaryMessageHandler;

Given that this is code that deals with async responses/message channels, are we introducing any lifetime issues by changing the third param to pass by reference rather than copy?

I'm actually sort of surprised that the compiler isn't shouting about a signature mismatch between the updated usage and definitions above given that it doesn't look like they were updated in this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartmorgan thoughts?

Copy link
Member

@cbracken cbracken Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, here's the scenario I'm thinking of:

  1. You're in some block of code; let's say a function somewhere. You have some locals.
  2. You construct a BinaryReply that captures (by reference) one or more of those locals. BinaryReply is a std::function so it can capture all it likes, unlike a function ptr.
  3. You construct a BinaryMessageHandler using that BinaryReply which is, as of this change, being passed by reference, and you put that somewhere to be used later.
  4. The scope in which you construct this thing ends and everything goes out of scope.

... time passes ...

  1. The message handler invokes the BinaryReply and tries to make use of the captured-by-ref local, which is gone now.

Copy link
Contributor

@stuartmorgan stuartmorgan Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a technical standpoint I don't think this changes anything in your scenario. Arguably the by-value signature better communicates that capturing by reference in the function is a bad idea though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that's a good point. Those captures are going to be equally broken in a by-copy scenario. I suspect the only code that'll be broken by this is code that's already quite racy and unlikely to exist in practice. The BinaryReply itself would need to go out of scope between the time the BinaryMessageHandler is invoked with it and the time it's actually used, which seems pretty unlikely in practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're saying slightly different things. My understanding is that there's no actual lifetime change here; AFAIK the by-value capture of the now-reference argument is still going to make a copy at that point.

// Use this channel's codec to decode the message and build a reply
// handler.
std::unique_ptr<T> message =
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/common/client_wrapper/include/flutter/event_channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class EventChannel {
// Mutable state to track the handler's listening status.
is_listening = bool(false)](const uint8_t* message,
const size_t message_size,
BinaryReply reply) mutable {
const BinaryReply& reply) mutable {
constexpr char kOnListenMethod[] = "listen";
constexpr char kOnCancelMethod[] = "cancel";

Expand Down
4 changes: 2 additions & 2 deletions shell/platform/common/client_wrapper/include/flutter/event_stream_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ struct StreamHandlerError {
const std::string error_message;
const std::unique_ptr<T> error_details;

StreamHandlerError(const std::string error_code,
const std::string error_message,
StreamHandlerError(const std::string& error_code,
const std::string& error_message,
matanlurey marked this conversation as resolved.
Show resolved Hide resolved
std::unique_ptr<T>&& error_details)
: error_code(error_code),
error_message(error_message),
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/common/client_wrapper/include/flutter/method_result_functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <functional>
#include <string>
#include <utility>

#include "method_result.h"

Expand Down Expand Up @@ -36,7 +37,7 @@ class MethodResultFunctions : public MethodResult<T> {
ResultHandlerNotImplemented<T> on_not_implemented)
: on_success_(on_success),
on_error_(on_error),
on_not_implemented_(on_not_implemented) {}
on_not_implemented_(std::move(on_not_implemented)) {}

virtual ~MethodResultFunctions() = default;

Expand Down
3 changes: 2 additions & 1 deletion shell/platform/common/client_wrapper/include/flutter/texture_registrar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <cstdint>
#include <functional>
#include <memory>
#include <utility>
#include <variant>

namespace flutter {
Expand All @@ -28,7 +29,7 @@ class PixelBufferTexture {
// take care of proper synchronization. It also needs to be ensured that the
// returned buffer isn't released prior to unregistering this texture.
explicit PixelBufferTexture(CopyBufferCallback copy_buffer_callback)
: copy_buffer_callback_(copy_buffer_callback) {}
: copy_buffer_callback_(std::move(copy_buffer_callback)) {}

// Returns the callback-provided FlutterDesktopPixelBuffer that contains the
// actual pixel data. The intended surface size is specified by |width| and
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/common/client_wrapper/testing/test_codec_extensions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class Point {
// variable-length type that includes types handled by the core standard codec.
class SomeData {
public:
SomeData(const std::string label, const std::vector<uint8_t>& data)
SomeData(const std::string& label, const std::vector<uint8_t>& data)
matanlurey marked this conversation as resolved.
Show resolved Hide resolved
: label_(label), data_(data) {}
~SomeData() = default;

Expand Down