Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3868/3/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 189: TEST_F(PartialRowTest, TestSetBinaryAndString) {
> Yes, it's a good idea to have a helper method for those. Will do.
Well, it's undefined behavior what happens to a freed string but depending on
ASAN seems fine to me. The ASAN build runs before each commit, and really all
the time.
Here is what happens with ASAN when you use freed memory:
1. take ptr to heap and store it
2. free that memory -> ASAN marks the memory region as poisoned
3. access the pointed-to memory -> ASAN intercepts the memory access and
crashes the test with a bunch of debugging information
Line 262: ASSERT_OK(row.SetStringNoCopy(2, src_str));
> SetXxxNoCopy() are the newly introduced methods, so I think it's nice to ha
ah, that's fair. I noticed that all the existing uses of SetString() were
converted to use SetStringNoCopy() in this patch so considered that general
test coverage, if not direct unit test coverage.
Line 276: EXPECT_EQ(reinterpret_cast<uintptr_t>(src_str.data()),
> I thought this part was derived from the interface constraints for the Slic
That's a reasonable argument, no worries, I'm not really against keeping this
test.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-HasComments: Yes
