Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15121 )
Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h@274 PS4, Line 274: class DefaultBlockBloomFilterBufferAllocator : public BlockBloomFilterBufferAllocatorIf { Did you look at gutil/singleton.h to simplify some of this? Might need to inherit enable_shared_from_this in order to convert a class instance into a shared_ptr. http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc@133 PS3, Line 133: } > What's the threshold for "using" directive? Good question; we haven't really defined one. The rule of thumb I use is "if you dropped the std:: prefix, is it still obvious that this class/function comes from the standard library?" For me the answer is yes with virtually all classes/collections, and "hmm maybe not" with really simple primitive functions (like swap, move, min, max, sort, etc.). http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc@270 PS4, Line 270: // Can't use std::make_shared<> due to private default constructor. We have a workaround for this in util/make_shared.h, which can work as long as you're OK making the constructor protected and not private. -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 4 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 05 Feb 2020 22:57:13 +0000 Gerrit-HasComments: Yes
