You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/03/26 22:23:31 UTC
[kudu-CR] util: helper class for working with bitsets
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12866
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 316 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/1
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc
File src/kudu/util/bitset-test.cc:
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@34
PS1, Line 34:
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@44
PS1, Line 44: typedef set<TestEnum> EnumSet;
> warning: 'MaxEnumVal' is a static definition in anonymous namespace; static
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@96
PS1, Line 96:
> Would be good to get some FindOr* coverage too.
FindOr* doesn't work for set types by virtue of them not being a pair. It also doesn't seem to have any value over using ContainsKey() for sets.
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@128
PS1, Line 128: // Do a final sanity check that the bitset looks how we expect.
> Check out https://github.com/google/googletest/blob/master/googletest/docs/
Forks :pepe-hands:
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@25
PS1, Line 25: #include <glog/logging.h>
> You should also talk about MaxVals; I think the "static" aspect of the Fixe
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@30
PS1, Line 30: s useful for building containers for enum types,
: // instead of usi
> Should we enforce this via COMPILE_ASSERT or the like?
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@39
PS1, Line 39: // compile time.
> Don't think you need the template arguments in the constructors.
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@43
PS1, Line 43: COMPILE_ASSERT(MaxVals < 64, bitset_size_too_large);
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@60
PS1, Line 60: template <typename Container>
> To make this more STL-like, should this return an integer indicating the nu
Done
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@66
PS1, Line 66:
> This isn't really an STL method; find() should be the primary implementatio
The implementation is easier with this method, not having to keep comparing against end() and whatnot. FWIW it will be in C++20. https://en.cppreference.com/w/cpp/container/set/contains
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 00:30:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#6).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 372 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/6
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 4: Verified+1
Unrelated failure of mt-tablet-test, which is known to be flaky.
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 03:07:33 +0000
Gerrit-HasComments: No
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#4).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 352 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/4
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@40
PS4, Line 40: size_t MaxVals
> I don't think so, I haven't seen a way that can do it without defining MaxV
Ah, we want to make sure 'non-existent' entries in enums are not accepted. Sure, that makes sense.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@46
PS4, Line 46: typedef IntType key_type;
> Yep, these are publicly needed for use in map-util.h
Whoops, it slipped down a couple of lines. I meant asking for COMPILE_ASSERT(MaxVals < 64, bitset_size_too_large);
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68
PS4, Line 68: const_iterator
> This is just to match the STL interface, http://www.cplusplus.com/reference
Well, at least make it iterator, not const_iterator if you want to mimic the STL interface? Otherwise I think it cannot be used by InsertOr* and alike: e.g., how is it supposed to update the value in case of InsertOrUpdate()? Does it work at all?
I would think of adding tests for right away if you are about introducing such entity, hoping it will fit those methods. Otherwise, why not to keep it private for a while?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@113
PS4, Line 113: IntType&
> I put these into godbolt https://godbolt.org/z/btGPzW and it seems compiler
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 18:02:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 04:55:05 +0000
Gerrit-HasComments: No
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt@a152
PS2, Line 152:
> What happened here?
Not sure, but it's already at L149?
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 00:43:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 5:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@33
PS4, Line 33: compile
> 'compile' or 'compilation'
Done
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@40
PS4, Line 40: size_t MaxVals
> Could MaxVals be deduced based on IntType? IntType stands for the enum's t
I don't think so, I haven't seen a way that can do it without defining MaxVals somehow (e.g as another enum value, or as a constexpr value).
Right, IntType is the enum type.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@46
PS4, Line 46: typedef IntType key_type;
> Is it necessary to have it in the public section? Maybe, leave it private?
Yep, these are publicly needed for use in map-util.h
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68
PS4, Line 68: const_iterator
> I'm curious when it's necessary to have a reference to the 'inserted' eleme
This is just to match the STL interface, http://www.cplusplus.com/reference/set/set/insert/
The iterator itself isn't particularly useful (for my purposes), but I matched the interface so this could be used with InsertOr*() methods which leverage it.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@105
PS4, Line 105: size_t size() const {
> const?
Done
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@113
PS4, Line 113: IntType&
> I guess in case of small enums using value instead of reference might be be
I put these into godbolt https://godbolt.org/z/btGPzW and it seems compilers are smart enough to generate the same code for small enums.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@131
PS4, Line 131: 'va
> What's 'i'?
Whoops, renamed it.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@184
PS4, Line 184: if (idx_ == MaxVals) {
> I'm not sure this is safe, even if this method is private. What happens if
Ah, good catch.
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 04:24:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 4:
(8 comments)
> Patch Set 3:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@33
PS4, Line 33: compiler
'compile' or 'compilation'
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@40
PS4, Line 40: size_t MaxVals
Could MaxVals be deduced based on IntType? IntType stands for the enum's type right?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@46
PS4, Line 46: typedef IntType key_type;
Is it necessary to have it in the public section? Maybe, leave it private?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68
PS4, Line 68: const_iterator
I'm curious when it's necessary to have a reference to the 'inserted' element?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@105
PS4, Line 105: size_t size() {
const?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@113
PS4, Line 113: IntType&
I guess in case of small enums using value instead of reference might be better.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@131
PS4, Line 131: 'i'
What's 'i'?
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@184
PS4, Line 184: while (++idx_ < MaxVals) {
I'm not sure this is safe, even if this method is private. What happens if accidentally calling this method twice and then trying to use the iterator?
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 03:51:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#2).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 352 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/2
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#7).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 373 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/7
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt@a152
PS2, Line 152:
What happened here?
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 00:34:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: util: helper class for working with bitsets
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Mar 2019 00:12:37 +0000
Gerrit-HasComments: No
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/3/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/3/src/kudu/util/bitset.h@53
PS3, Line 53: FixedBitSet(std::initializer_list<IntType> list) {
> warning: initializer-list constructor should not be declared explicit [goog
Done
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 01:29:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@40
PS4, Line 40:
> Ah, we want to make sure 'non-existent' entries in enums are not accepted.
Yeah, I'll add a comment since that isn't clear.
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@46
PS4, Line 46: class iterator;
> Whoops, it slipped down a couple of lines. I meant asking for COMPILE_ASSE
Done
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68
PS4, Line 68: 'val' into th
> Well, at least make it iterator, not const_iterator if you want to mimic th
AFAICT set<> types don't support InsertOrUpdate(), at least when I try to compile:
std::unordered_set<int> test;
InsertOrUpdate(&test, 0);
It's unclear what the point of InsertOrUpdate() would be for set types anyway, vs InsertIfNotPresent() or InsertOrDie(), which both work and are tested. Same with FindOr* functions, those functions don't work with sets and ContainsKey() is preferred anyway.
That said, I've updated to use iterator instead of const_iterator just for parity with STL's interface, but nothing that works from map-util uses const_iterator AFAICT. Also added a test to test the output of insert().
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 22:09:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68
PS4, Line 68: 'val' into th
> AFAICT set<> types don't support InsertOrUpdate(), at least when I try to c
Yep, I think InsertOrUpdate() doesn't make sense for set types: updating the key which is being inserted is kind of nonsense.
All right, thank you for addressing my concerns and adding corresponding test.
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 28 Mar 2019 00:19:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc
File src/kudu/util/bitset-test.cc:
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@96
PS1, Line 96: ASSERT_EQ(InsertIfNotPresent(&enum_set, e),
Would be good to get some FindOr* coverage too.
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@128
PS1, Line 128: TEST(TestBitSet, TestInvalidUsage) {
Check out https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#death-test-naming
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h
File src/kudu/util/bitset.h:
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@25
PS1, Line 25: // Utility template for working with a bitset to make it feel more like a
You should also talk about MaxVals; I think the "static" aspect of the FixedBitSet is possibly it's most important aspect (i.e. you need to know the size of the bitset at compile time to use this).
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@30
PS1, Line 30: so the expectation is that 'MaxVals' is smaller than
: // the word-size.
Should we enforce this via COMPILE_ASSERT or the like?
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@39
PS1, Line 39: FixedBitSet<IntType, MaxVals>() {}
Don't think you need the template arguments in the constructors.
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@60
PS1, Line 60: void erase(const IntType val) {
To make this more STL-like, should this return an integer indicating the number of values erased (i.e. 0 or 1)?
http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@66
PS1, Line 66: bool contains(const IntType& val) const {
This isn't really an STL method; find() should be the primary implementation, at which point the gutil/map-util.h methods ought to work as wrappers.
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 26 Mar 2019 22:39:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#3).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 352 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/3
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Reviewed-on: http://gerrit.cloudera.org:8080/12866
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 373 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] util: helper class for working with bitsets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/12866/2/src/kudu/util/CMakeLists.txt@a152
PS2, Line 152:
> Not sure, but it's already at L149?
Oh weird. I didn't think to check for duplicates.
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 27 Mar 2019 03:25:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: helper class for working with bitsets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12866
to look at the new patch set (#5).
Change subject: util: helper class for working with bitsets
......................................................................
util: helper class for working with bitsets
Adds a templatized helper class to facilitate working with bitsets. This
is just a wrapper around std::bitset, but it exposes a more
container-like interface. This is particularly useful for specifying
containers of enum types and the like (e.g. rather than using a hashed
container).
This also adds an iterator class for the new wrapper; such an iterator
apparently doesn't exist[1] in the STL bitset class.
[1] https://stackoverflow.com/a/34728458
Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/bitset-test.cc
A src/kudu/util/bitset.h
3 files changed, 355 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/12866/5
--
To view, visit http://gerrit.cloudera.org:8080/12866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4
Gerrit-Change-Number: 12866
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)