-
Notifications
You must be signed in to change notification settings - Fork 82
refactor: drop rvalue from link::IChannel interface #571
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.
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @tongke6, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the link::IChannel
interface to use pass-by-value for Buffer
sink parameters instead of r-value references. This is a positive change that aligns with modern C++ best practices, making the API more flexible by allowing both l-values (via copy) and r-values (via move) to be passed. The changes have been applied consistently across the interface, mock objects, and concrete implementations. My review includes a couple of suggestions to remove redundant std::move
calls on temporary objects, which will improve code clarity and adherence to idiomatic C++.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
} | ||
|
||
virtual void SendAsync(const std::string& key, Buffer&& value) = 0; | ||
virtual void SendAsync(const std::string& key, Buffer value) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
移除 rvalue interface 的原因是 pybind 不支持移动语义吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
支持得不太好。另外,在接口里暴露 rr value 也不是好的设计。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
现在 ByteContainerView 为参数的 SendAsync 是通过调用 Buffer&& 为参数的 SendAsync 实现的。
而 Buffer 的拷贝是深拷贝(作为RAII Object,也确实需要是深拷贝)。
似乎有效率问题
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
很好的问题,我最开始在里面调用的时候,显示第手写了 std::move,但是 gemini 还是 copolit 提醒我是不必要的,编译器会自动给临时对象添加 std::move,避免拷贝。
这个问题可以写个 demo 快速地验证下。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
很好的问题,我最开始在里面调用的时候,显示第手写了 std::move,但是 gemini 还是 copolit 提醒我是不必要的,编译器会自动给临时对象添加 std::move,避免拷贝。
这个问题可以写个 demo 快速地验证下。
验过了,gemini是对的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
写了一个 example,如 gemin 所说,直接构造了 Buffer 参数,都没有调用 移动构造函数和拷贝构造函数。https://godbolt.org/z/EK5EKvjqa
No description provided.