github-actions[bot] commented on code in PR #25788:
URL: https://github.com/apache/doris/pull/25788#discussion_r1368435166
##########
be/src/util/bitmap_value.h:
##########
@@ -519,49 +519,42 @@ class Roaring64Map {
// we cannot use operator == on the map because either side may contain
// empty Roaring Bitmaps
auto lhs_iter = roarings.cbegin();
+ auto lhs_cend = roarings.cend();
auto rhs_iter = r.roarings.cbegin();
- do {
- // if the left map has reached its end, ensure that the right map
- // contains only empty Bitmaps
- if (lhs_iter == roarings.cend()) {
- while (rhs_iter != r.roarings.cend()) {
- if (rhs_iter->second.isEmpty()) {
- ++rhs_iter;
- continue;
- }
- return false;
- }
- return true;
- }
- // if the left map has an empty bitmap, skip it
- if (lhs_iter->second.isEmpty()) {
+ auto rhs_cend = r.roarings.cend();
+ while (lhs_iter != lhs_cend && rhs_iter != rhs_cend) {
+ auto lhs_key = lhs_iter->first, rhs_key = rhs_iter->first;
Review Comment:
warning: multiple declarations in a single statement reduces readability
[readability-isolate-declaration]
```suggestion
auto lhs_key = lhs_iter->first;
auto rhs_key = rhs_iter->first;
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
+ detail::Roaring64Map roaring64_map1, roaring64_map2;
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.add(uint32_t(100));
Review Comment:
warning: 100 is a magic number; consider replacing it with a named constant
[readability-magic-numbers]
```cpp
roaring64_map1.add(uint32_t(100));
^
```
##########
be/src/util/bitmap_value.h:
##########
@@ -519,49 +519,42 @@ class Roaring64Map {
// we cannot use operator == on the map because either side may contain
// empty Roaring Bitmaps
auto lhs_iter = roarings.cbegin();
+ auto lhs_cend = roarings.cend();
auto rhs_iter = r.roarings.cbegin();
- do {
- // if the left map has reached its end, ensure that the right map
- // contains only empty Bitmaps
- if (lhs_iter == roarings.cend()) {
- while (rhs_iter != r.roarings.cend()) {
- if (rhs_iter->second.isEmpty()) {
- ++rhs_iter;
- continue;
- }
- return false;
- }
- return true;
- }
- // if the left map has an empty bitmap, skip it
- if (lhs_iter->second.isEmpty()) {
+ auto rhs_cend = r.roarings.cend();
+ while (lhs_iter != lhs_cend && rhs_iter != rhs_cend) {
+ auto lhs_key = lhs_iter->first, rhs_key = rhs_iter->first;
+ const auto &lhs_map = lhs_iter->second, &rhs_map =
rhs_iter->second;
Review Comment:
warning: multiple declarations in a single statement reduces readability
[readability-isolate-declaration]
```suggestion
const auto &lhs_map = lhs_iter->second;
const auto &rhs_map = rhs_iter->second;
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
Review Comment:
warning: all parameters should be named in a function
[readability-named-parameter]
```suggestion
TEST(BitmapValueTest /*unused*/, Roaring64Map_operator_eq /*unused*/) {
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
+ detail::Roaring64Map roaring64_map1, roaring64_map2;
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.remove(uint32_t(100));
Review Comment:
warning: 100 is a magic number; consider replacing it with a named constant
[readability-magic-numbers]
```cpp
roaring64_map1.remove(uint32_t(100));
^
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
