ci: fix clang tidy after PR 2769 #2799
Conversation
canepat
force-pushed
the
ci/fix_clang_tidy_after_2769
branch
from
1cfb149 to
e436224
Compare
| @@ -61,7 +61,7 @@ struct DomainRangeLatestQuery { | |||
| return decode_kv_pair(kv_pair); | |||
| }; | |||
|
|
|||
| auto exec_with_eager_begin(Bytes key_start, Bytes key_end, bool ascending) { | |||
| auto exec_with_eager_begin(const Bytes& key_start, Bytes key_end, bool ascending) { | |||
There was a problem hiding this comment.
I recommend to add NOLINT here or pack keys into std::pair in order to preserve symmetry.
| @@ -101,12 +101,12 @@ struct DomainRangeLatestQuery { | |||
| TKeyEncoder key_start_encoder; | |||
| key_start_encoder.value = key_start; | |||
| Slice key_start_slice = key_start_encoder.encode(); | |||
| Bytes key_start_data = Bytes{from_slice(key_start_slice)}; | |||
| Bytes key_start_data = Bytes{from_slice(key_start_slice)}; // TODO(canepat) extract data from encoder instead of copying | |||
There was a problem hiding this comment.
I didn't bother about this copy, because there's a constant overhead per request (2 copies per request), and keys are not huge. I suggest to not add TODO comments about optimization unless we identify a bottleneck here.
Note that encoders not always contain Bytes inside, so another approach would be required for eliminating this copy.
| @@ -72,7 +72,7 @@ struct HistoryRangeByKeysQuery { | |||
| }; | |||
| } | |||
|
|
|||
| auto exec_with_eager_begin(Bytes key_start, Bytes key_end, Timestamp timestamp, bool ascending) { | |||
| auto exec_with_eager_begin(const Bytes& key_start, Bytes key_end, Timestamp timestamp, bool ascending) { | |||
There was a problem hiding this comment.
I recommend to add NOLINT here or pack keys into std::pair in order to preserve symmetry.
| } | ||
| if (key_decoder.value.timestamp.value < ts_range.end) { |
There was a problem hiding this comment.
I'm not a fan of this lint rule. It is not clear what does it improve for us. I feel that the code is less readable and less robust.
| @@ -46,7 +46,7 @@ struct DomainRangeLatestSegmentQuery { | |||
|
|
|||
| using ResultItem = btree::BTreeIndex::Cursor::value_type; | |||
|
|
|||
| auto exec_with_eager_begin(Bytes key_start, Bytes key_end, bool ascending) { | |||
| auto exec_with_eager_begin(const Bytes& key_start, Bytes key_end, bool ascending) { | |||
There was a problem hiding this comment.
I recommend to add NOLINT here or pack keys into std::pair in order to preserve symmetry.
| @@ -57,7 +57,7 @@ struct DomainRangeLatestSegmentQuery { | |||
|
|
|||
| auto exec(Bytes key_start, Bytes key_end, bool ascending) { | |||
| auto exec_func = [query = *this, key_start = std::move(key_start), key_end = std::move(key_end), ascending]() mutable { | |||
| return query.exec_with_eager_begin(std::move(key_start), std::move(key_end), ascending); | |||
| return query.exec_with_eager_begin(key_start, std::move(key_end), ascending); | |||
There was a problem hiding this comment.
If you decide to go with const Bytes& key_start I'd still move it for symmetry as you do in other 2 places unless it produces a warning (in which case need to fix 2 other places).
| @@ -70,7 +70,7 @@ struct HistoryRangeByKeysSegmentQuery { | |||
| return std::pair{std::move(key_data), std::move(*value_opt)}; | |||
| } | |||
|
|
|||
| auto exec_with_eager_begin(Bytes key_start, Bytes key_end, datastore::Timestamp timestamp, bool ascending) { | |||
| auto exec_with_eager_begin(const Bytes& key_start, Bytes key_end, datastore::Timestamp timestamp, bool ascending) { | |||
There was a problem hiding this comment.
I recommend to add NOLINT here or pack keys into std::pair in order to preserve symmetry.
| @@ -94,7 +94,7 @@ struct HistoryRangeByKeysSegmentQuery { | |||
|
|
|||
| auto exec(Bytes key_start, Bytes key_end, datastore::Timestamp timestamp, bool ascending) { | |||
| auto exec_func = [query = *this, key_start = std::move(key_start), key_end = std::move(key_end), timestamp, ascending]() mutable { | |||
| return query.exec_with_eager_begin(std::move(key_start), std::move(key_end), timestamp, ascending); | |||
| return query.exec_with_eager_begin(key_start, std::move(key_end), timestamp, ascending); | |||
There was a problem hiding this comment.
If you decide to go with const Bytes& key_start I'd still move it for symmetry as you do in other 2 places unless it produces a warning (in which case need to fix 2 other places).
No description provided.