PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1402912187
##########
src/common/lock_manager.h:
##########
@@ -81,6 +81,8 @@ class LockManager {
class LockGuard {
public:
+ LockGuard() = default;
Review Comment:
This ctor will make the state of a LockGuard object more complicated, which
is not what we want in my view.
You can refer to
https://en.cppreference.com/w/cpp/thread/lock_guard/lock_guard that also
doesn't provide such a ctor.
And if you do not want to initialize the object when you define it, please
use `std::optional`
##########
src/common/lock_manager.h:
##########
@@ -94,8 +96,10 @@ class LockGuard {
LockGuard(LockGuard &&guard) : lock_(guard.lock_) { guard.lock_ = nullptr; }
+ LockGuard &operator=(LockGuard &&) = default;
Review Comment:
IMHO it's not a well-formed move assignment operator, which can lead to
double unlock.
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
+
decltype(auto) RawTake() { return *begin_++; }
decltype(auto) RawNext() { ++begin_; }
bool Good() const { return begin_ != end_; }
+ std::enable_if_t<IsRandomAccessIter(), size_t> Remains() const {
+ // O(1) iff Iter is random access iterator.
+ auto d = std::distance(begin_, end_);
+ if (d < 0) {
+ d = 0;
+ }
Review Comment:
I think maybe we can just DCHECK or assert here, then we can find problem
when we encounter an invalid state.
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
Review Comment:
I think you can use something like std::advance here to avoid the
requirement of random access iterator.
##########
src/commands/command_parser.h:
##########
@@ -41,6 +41,10 @@ struct CommandParser {
public:
using ValueType = typename Iter::value_type;
+ static constexpr bool IsRandomAccessIter() noexcept {
+ return std::is_base_of_v<std::random_access_iterator_tag, typename
std::iterator_traits<Iter>::iterator_category>;
+ }
Review Comment:
```suggestion
static constexpr bool IsRandomAccessIter =
std::is_base_of_v<std::random_access_iterator_tag, typename
std::iterator_traits<Iter>::iterator_category>;
```
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
+
decltype(auto) RawTake() { return *begin_++; }
decltype(auto) RawNext() { ++begin_; }
bool Good() const { return begin_ != end_; }
+ std::enable_if_t<IsRandomAccessIter(), size_t> Remains() const {
+ // O(1) iff Iter is random access iterator.
+ auto d = std::distance(begin_, end_);
+ if (d < 0) {
+ d = 0;
+ }
+ return d;
+ }
+
+ size_t Eat(size_t count) {
Review Comment:
```suggestion
size_t Skip(size_t count) {
```
##########
src/common/parse_util.h:
##########
@@ -53,13 +58,18 @@ struct ParseIntFunc<long long> { // NOLINT
constexpr static const auto value = std::strtoll;
};
+template <>
+struct ParseIntFunc<unsigned char> { // NOLINT
+ constexpr static const auto value = std::strtol;
Review Comment:
```suggestion
constexpr static const auto value = std::strtoul;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
