[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3595/3/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 707: EraseKeyReturnSmartPtrValue(Collection* const collection,
this is more generic than just smart pointer, right? in fact, this is identical 
to the above function except for doing 'std::move' and relying on the default 
constructor instead of just using 'NULL'.

In other words, I think your new function implements the original function too, 
so if you just changed the original to do what you're doing here, you could use 
it in both cases without adding new code?


-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3595/3/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 711: return typename Collection::value_type::second_type();
It seems I missed it first time, but anyways: consider replacing 
Collection::value_type::second_type with Collection::mapped_type here.


Line 713:   typename Collection::value_type::second_type value = 
std::move(it->second);
Ditto: consider changing Collection::value_type::second_type --> 
Collection::mapped_type for better readability.


-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 3: Code-Review+1

just a rebase, keeping alexey's +1

-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2246/

-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2239/

-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3595

to look at the new patch set (#2).

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..

Add a EraseKeyReturnSmartPtrValue to map-util

This adds a new method to map-util to help in erasing key/value pairs for
containers whose value is of smart pointer type.

Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
---
M src/kudu/gutil/map-util.h
M src/kudu/util/map-util-test.cc
2 files changed, 43 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/3595/2
-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 700: // EraseKeyReturnSmartPtrValue(_map, "my_key, _value);
A tiny typo: "my_key -- > "my_key"


Line 701: template 
What if the collection is a multi-key container, like multimap?  Is it OK just 
to remove the first element having that key and leave the rest?  If so, please 
add explanation about that in the comment for the function.


Line 702: void EraseKeyReturnSmartPtrValue(Collection* const collection,
Consider returning the result as a smart pointer instead of placing it into the 
parameter placeholder.  From the caller's perspective that would be more terse 
then:

unique_ptr p(EraseKeyReturnSmartPtrValue(my_map, "key");

Also, why not to pass input collection by const reference?  Passing parameter 
by pointer means it might be null, so it's necessary to take care of that.


Line 703:  const typename 
Collection::value_type::first_type& key,
Why not just Collection::key_type, but Collection::value_type::first_type?


http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

Line 80: TEST(EraseKeyReturnSmartPtrValueTest, TestEraseKeyReturnSmartPtrValue) 
{
Does it make sense to add a testcase for multi-key dictionary like multimap?


-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes