GEODE-3288: Converts char* to std::string on public API. by jake-at-work · Pull Request #160 · apache/geode-native

jake-at-work · 2017-11-29T01:30:12Z

Excludes DataInput/Output, Serializer and CacheableString.

jake-at-work · 2017-11-29T03:33:53Z

        }

-        string locatorArgs = LocatorStartArgs + " --name=" + serverName + startDir + extraLocatorArgs;
+        string locatorArgs = LocatorStartArgs + " --name=" + serverName + startDir + extraLocatorArgs + " --http-service-port=0";


This change shaves about 10s off each test!

jake-at-work · 2017-11-29T03:35:33Z

+)
+
+include(PrecompiledHeader)
+add_precompiled_header(${PROJECT_NAME} geode_includes.hpp FORCEINCLUDE)


Added precompiled headers to work around and speed up issue including marshal_as headers.

dgkimura

Looks good. Just small comments so feel free to ignore as you see fit. I did notice you changed many types to universal refs auto&&, which I assume was intentional. How did you decide on using auto& vs const auto& vs auto&&?

dgkimura · 2017-11-29T15:12:44Z

 string(REPLACE "/EHsc" "/EHa" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
 string(REPLACE "/RTC1" "" CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG}")
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /clr /wd4947 /wd4251 /wd4635 /doc /we4488")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /clr /bigobj /wd4947 /wd4251 /wd4635 /doc /we4488")


I'm a little worried about remembering the context of these flags and why they're required.

Good point, though if we remove it the project won't link, so context will be re-established. ;) Would you like a comment?

I would find a comment to be helpful

dgkimura · 2017-11-29T15:27:14Z

   */
-  void setEndpoints(char* endpoints);
+  void setEndpoints(const std::string& endpoints);
+  void setEndpoints(std::string&& endpoints);


Did you implement an rvalue ref version of this function? If so, why was it needed?

I think I was experimenting here. I should back it out. We can always add it later.

void setEndpoints(const std::string& endpoints);
Does not result in a copy to make the call but since internally we keep it a copy is made. No optimization can be made but none is really necessary unless the param is an r-value. The method also fails to communicate that we are keeping a copy.

void setEndpoints(std::string endpoints);
Communicates we are keeping a copy as copy is don't on the call. Unoptimized the copy happens also at the assignment to the member variable, though most modern compilers I believe will optimize that second copy out.

void setEndpoints(std::string&& endpoints);
Communicates a copy is kept and optimizes the r-value to avoid extra copies.

The two methods implemented are consistent with what the container class templates generate to handle l-values and r-values optimally.

dgkimura · 2017-11-29T15:29:55Z

-
 /** Template CacheableKey class for primitive types. */
 template <typename TObj, int8_t TYPEID, const char* TYPENAME,
          const char* SPRINTFSYM, int32_t STRSIZE>


Do we want to change these template arguments from char* to std::string as well?

I don't think we can, std::string is not constexpr. C++17 adds std::string_view which is constexpr.

dgkimura · 2017-11-29T15:49:17Z

@@ -219,8 +219,9 @@ void QueryHelper::populatePortfolioPdxData(std::shared_ptr<Region>& rptr,
      auto keyport = CacheableKey::create(portname);

      rptr->put(keyport, port);
-      LOGINFO("populatePortfolioPdxData:: Put for iteration current = %d done",
-              current);
+      // LOGINFO("populatePortfolioPdxData:: Put for iteration current = %d


Intentional comment?

Yes, it fills the logs. I should probably just delete but for diagnostics someone may want it.

Maybe LOGDEBUG is a good compromise?

dgkimura · 2017-11-29T15:51:24Z

-               bool pisLargeResultset = false) {
-    size_t querylen = static_cast<int>(strlen(pquery));
-    if (querylen < MAX_QRY_LENGTH) memcpy(_query, pquery, querylen);
-    memset(&_query[querylen], '\0', 1);


Yay! Glad to see these converted.

dgkimura · 2017-11-29T16:05:26Z

@@ -298,32 +305,6 @@ CacheableString::~CacheableString() {
  }
 }

-int32_t CacheableString::logString(char* buffer, int32_t maxLength) const {


dgkimura · 2017-11-29T16:07:16Z

-        if (bucketId == -1) {
-          return;
-        }
+      auto&& partition = fpResolver->getPartitionName(event);


Same comment as above about intent of universal refs here.

dgkimura · 2017-11-29T16:12:40Z

+    return names[ordinal];
+  }
+
+  return names[NONE];


Ugh.. I realize you didn't introduce this, but I'm not really a fan of using enum types as integers in c++.

I agree. Can you open a ticket and we can investigate alternatives?

Created ticket here: https://issues.apache.org/jira/browse/GEODE-4032

dgkimura · 2017-11-29T16:15:14Z

  if (INVALIDATE <= ordinal && ordinal <= LOCAL_DESTROY) {
    return names[ordinal];
  }
-  return nullptr;
+  return names[INVALID_ACTION];


Same as other comment regarding enums

dgkimura · 2017-11-29T16:38:38Z

      }
    }

-    cacheImpl->getPoolManager().addPool(name,
+    cacheImpl->getPoolManager().addPool(std::move(name),


Why move?

Why not? The method is PoolFactory::create(std::string name) so I am moving the already copied value into the member variable, rather than another copy. Saves the copy of the underlying char[].

I feel like it makes it just a little easier to shoot ourselves in the foot (e.g. accidentally editing to use name after this line). Also since PoolFactory::create probably won't be called in a tight loop a copy doesn't seem too terrible?

jake-at-work · 2017-11-29T23:06:23Z

@dgkimura Can I get a final blessing?

Excludes DataInput/Output, Serializer and CacheableString.

- Reduces integration test runtimes by not starting http service in locators. - Fixes some const method declarations.

jake-at-work requested a review from dgkimura November 29, 2017 01:30

jake-at-work force-pushed the feature/GEODE-3288 branch 2 times, most recently from 9658b9f to edf10d0 Compare November 29, 2017 03:30

jake-at-work commented Nov 29, 2017

View reviewed changes

dgkimura approved these changes Nov 29, 2017

View reviewed changes

jake-at-work added 3 commits December 1, 2017 13:28

GEODE-3288: Converts char* to std::string on public API.

309e4d7

Excludes DataInput/Output, Serializer and CacheableString.

GEODE-3288: Minor adjustments based on feedback.

1c4aa41

GEODE-3288: Rebase corrections.

3e3f26f

jake-at-work force-pushed the feature/GEODE-3288 branch from 689f8de to 3e3f26f Compare December 1, 2017 23:14

jake-at-work merged commit b1d36ed into apache:develop Dec 2, 2017

mmartell pushed a commit to mmartell/geode-native that referenced this pull request Mar 6, 2018

GEODE-3288: Converts char* to std::string on public API. (apache#160)

4b596a4

- Reduces integration test runtimes by not starting http service in locators. - Fixes some const method declarations.

mmartell pushed a commit to mmartell/geode-native that referenced this pull request Mar 6, 2018

GEODE-3288: Converts char* to std::string on public API. (apache#160)

1ec3638

- Reduces integration test runtimes by not starting http service in locators. - Fixes some const method declarations.

pdxcodemonkey mentioned this pull request Mar 30, 2021

GEODE-8543: Fix for uneccessary connections closure #705

Merged

Uh oh!

Conversation

jake-at-work commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgkimura left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jake-at-work commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants