-
Notifications
You must be signed in to change notification settings - Fork 142
added explicit ref-qualifiers for Error::what() and a additional over… #505
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
added explicit ref-qualifiers for Error::what() and a additional over… #505
Conversation
…load to allow moving Error::what_ to caller if Error is an rvalue
Summary of ChangesHello @BestITUserEUW, 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 enhances 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
|
@liuzicheng1987 Do you want me to setup tests for Error::what() ? |
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 introduces ref-qualified overloads for Error::what()
, which is a great improvement for allowing moves from rvalue Error
objects, enhancing both performance and safety. My review includes a suggestion to add a const &&
overload to fully support all value categories and prevent a potential compilation failure with const
rvalue Error
objects.
const std::string& what() const & { return what_; } | ||
/// Moves the error message out of Error object and leaves what_ in a moved from state | ||
std::string what() && { return std::move(what_); } |
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.
The introduction of lvalue- and rvalue-qualified overloads for what()
is a great improvement for both safety and performance. However, the current implementation has a small gap: it doesn't handle const
rvalues.
For example, code like this would fail to compile with the current changes:
const rfl::Error make_const_error() { return rfl::Error("an error"); }
// ...
auto msg = make_const_error().what(); // Fails to compile
This is because make_const_error()
returns a const
rvalue. The &&
overload of what()
is not const
-qualified, and the const &
overload requires an lvalue, so neither can be called.
To fix this and support const
rvalues, you could add a const &&
overload. This overload would behave like the const &
one, returning a const
reference to the string, which is safe. This would make the interface for what()
complete with respect to value categories.
const std::string& what() const & { return what_; }
/// Moves the error message out of Error object and leaves what_ in a moved from state.
std::string what() && { return std::move(what_); }
/// Returns the error message from a const rvalue.
const std::string& what() const && { return what_; }
@BestITUserEUW , no, I think that would be overkill. Thanks for your contribution! |
…load to allow moving Error::what_ to caller if Error is an rvalue. Adresses #504