[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #703: GEODE-8756: Fix CacheableString::objectSize

[GitBox <gi...@apache.org>]

pivotal-jbarrett commented on a change in pull request #703:
URL: https://github.com/apache/geode-native/pull/703#discussion_r544465839



##########
File path: cppcache/src/CacheableString.cpp
##########
@@ -119,8 +119,18 @@ bool CacheableString::isAscii(const std::string& str) {
 }
 
 size_t CacheableString::objectSize() const {
-  auto size = sizeof(CacheableString) +
-              sizeof(std::string::value_type) * m_str.capacity();
+  auto size = sizeof(CacheableString);
+
+  // This is calculated in order not to count more bytes than necessary
+  // whenever SSO applies.
+  auto delta = reinterpret_cast<const uint8_t*>(m_str.data()) -
+               reinterpret_cast<const uint8_t*>(&m_str);
+  if (delta >= static_cast<decltype(delta)>(sizeof(decltype(m_str))) ||
+      delta < 0) {
+    // Add an extra character for the null-terminator
+    size += sizeof(decltype(m_str)::value_type) * (m_str.capacity() + 1UL);

Review comment:
       Very subtle find on the null terminator!
   > Memory locations obtained from the allocator but not available for storing any element are not counted in the allocated storage. Note that the null terminator is not an element of the basic_string.

##########
File path: cppcache/test/CacheableStringTests.cpp
##########
@@ -224,4 +224,28 @@ TEST_F(CacheableStringTests, TestFromDataNonAsciiHuge) {
   EXPECT_EQ(utf8, str->value());
 }
 
+class CacheableStringSizeTests : public ::testing::TestWithParam<std::size_t> {
+};
+
+INSTANTIATE_TEST_SUITE_P(CacheableStringTests, CacheableStringSizeTests,
+                         ::testing::Values(0UL, 1UL, 3UL, 7UL, 15UL, 23UL, 31UL,
+                                           47UL, 63UL, 1025UL, 4097UL));
+
+TEST_P(CacheableStringSizeTests, TestObjectSize) {
+  auto size = GetParam();
+  std::string str(size, '_');
+  auto cacheable = CacheableString::create(str);
+  EXPECT_TRUE(cacheable);
+
+  auto expected_size = sizeof(CacheableString);

Review comment:
       If we are literally copying the internal logic of the method is it really a valid test? Is there another way test the response?
   
   Could we provide a customized allocator that we use for checking allocations? 🤷 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org