[GitHub] [geode-native] mreddington commented on a change in pull request #667: GEODE-8531: Fix coredump in MapSegment::remove

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

mreddington commented on a change in pull request #667:
URL: https://github.com/apache/geode-native/pull/667#discussion_r500527381



##########
File path: cppcache/integration/test/RegisterKeysTest.cpp
##########
@@ -46,9 +46,11 @@ Cache createTestCache() {
       .create();
 }
 
-std::shared_ptr<Region> setupCachingProxyRegion(Cache& cache) {
+std::shared_ptr<Region> setupCachingProxyRegion(Cache& cache,

Review comment:
       Prefer an overload over a default parameter. This obfuscates the code as documentation and the meaning of all the prior tests. You're explicitly stating concurrency checks are not to be enabled. Why is that an important condition for the other tests? I presume what you wanted was the ability to enable checks for your one test.




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