-
-
Notifications
You must be signed in to change notification settings - Fork 861
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] Code cleanup #7699
[base] Code cleanup #7699
Conversation
4cc4e37
to
7b55dea
Compare
Linux check is failing. @vng I would also like to see a clean code in CLion Nova. |
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.
lgtm
@@ -71,7 +74,7 @@ template <class T, size_t N> class buffer_vector | |||
|
|||
buffer_vector(buffer_vector const &) = default; | |||
|
|||
buffer_vector(buffer_vector && rhs) : m_size(rhs.m_size), m_dynamic(std::move(rhs.m_dynamic)) | |||
buffer_vector(buffer_vector && rhs) noexcept : m_size(rhs.m_size), m_dynamic(std::move(rhs.m_dynamic)) |
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.
Noexceptness here and below should probably depend on T.
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.
Well, yes, actually it should depend on T(T&&) noexcept
- Simple one: add
static_assert(noexcept(T()));
- Complex one: add
enable_if blah-blah
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.
noexcept(std::is_nothrow_constructible_v<T, T>)
?
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.
Looks good. Does it check T() and T(T&&) ?
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.
I'm not sure about noexcept(std::is_nothrow_constructible_v<T, T>)
I'd better revert this change, because I don't have time to investigate it right now
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.
ctor and operator= both use MoveStatic, which uses std::move algo, so should be enough:
static_assert(std::is_nothrow_move_assignable<T>::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.
I did and pushed it myself.
@@ -71,7 +74,7 @@ template <class T, size_t N> class buffer_vector | |||
|
|||
buffer_vector(buffer_vector const &) = default; | |||
|
|||
buffer_vector(buffer_vector && rhs) : m_size(rhs.m_size), m_dynamic(std::move(rhs.m_dynamic)) | |||
buffer_vector(buffer_vector && rhs) noexcept : m_size(rhs.m_size), m_dynamic(std::move(rhs.m_dynamic)) |
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.
Well, yes, actually it should depend on T(T&&) noexcept
- Simple one: add
static_assert(noexcept(T()));
- Complex one: add
enable_if blah-blah
9da7570
to
7c69319
Compare
@@ -81,7 +84,7 @@ template <class T, size_t N> class buffer_vector | |||
|
|||
buffer_vector & operator=(buffer_vector const & rhs) = default; | |||
|
|||
buffer_vector & operator=(buffer_vector && rhs) | |||
buffer_vector & operator=(buffer_vector && rhs) noexcept |
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.
If you reverted ctor, remove here also. They both use MoveStatic function.
Signed-off-by: Andrew Shkrob <andrew.shkrob.social@yandex.by>
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.
Thanks for the cleanup!
Removed includes
Removed namespaces
Fixed formatting (empty lines, template functions)
These changes are based on clang-tidy in IDE