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



##########
File path: cppcache/src/CacheXmlParser.cpp
##########
@@ -562,27 +555,15 @@ void CacheXmlParser::startCache(const xercesc::Attributes 
&attrs) {
     }
   }
 
-  _GEODE_NEW(cacheCreation_, CacheXmlCreation());
+  cacheCreation_ = make_unique<CacheXmlCreation>();
 }
 
 void CacheXmlParser::startPdx(const xercesc::Attributes &attrs) {
-  auto ignoreUnreadFields = getOptionalAttribute(attrs, IGNORE_UNREAD_FIELDS);
-  if (!ignoreUnreadFields.empty()) {
-    if (equal_ignore_case(ignoreUnreadFields, "true")) {
-      cacheCreation_->setPdxIgnoreUnreadField(true);
-    } else {
-      cacheCreation_->setPdxIgnoreUnreadField(false);
-    }
-  }
-
-  auto pdxReadSerialized = getOptionalAttribute(attrs, READ_SERIALIZED);
-  if (!pdxReadSerialized.empty()) {
-    if (equal_ignore_case(pdxReadSerialized, "true")) {
-      cacheCreation_->setPdxReadSerialized(true);
-    } else {
-      cacheCreation_->setPdxReadSerialized(false);
-    }
-  }
+  CacheRegionHelper::getCacheImpl(cache_)->setPdxIgnoreUnreadFields(

Review comment:
       Are you certain this logic does not override the default value of 
property. The original code only change the value if the values was provided in 
the XML, preserving the default value in the object. 

##########
File path: cppcache/src/CacheXmlCreation.cpp
##########
@@ -34,37 +35,21 @@ void 
CacheXmlCreation::addPool(std::shared_ptr<PoolXmlCreation> pool) {
   pools.push_back(pool);
 }
 
-void CacheXmlCreation::create(Cache* cache) {
-  m_cache = cache;
-  m_cache->m_cacheImpl->setPdxIgnoreUnreadFields(m_pdxIgnoreUnreadFields);
-  m_cache->m_cacheImpl->setPdxReadSerialized(m_readPdxSerialized);
+void CacheXmlCreation::create(Cache& cache) {
   // Create any pools before creating any regions.
-
-  for (const auto& pool : pools) {
-    pool->create();
-  }
-
-  for (const auto& rootRegion : rootRegions) {
-    rootRegion->createRoot(cache);
-  }
-}
-
-void CacheXmlCreation::setPdxIgnoreUnreadField(bool ignore) {
-  // m_cache->m_cacheImpl->setPdxIgnoreUnreadFields(ignore);
-  m_pdxIgnoreUnreadFields = ignore;
+  std::for_each(std::begin(pools), std::end(pools),
+                std::bind(&PoolXmlCreation::create,
+                          std::bind(&std::shared_ptr<PoolXmlCreation>::get,
+                                    std::placeholders::_1)));

Review comment:
       Hmm... Does this modern C++ style make this any more readable?
   
   Would dereferencing the `shared_ptr`s as local variables prior to these 
calls help with readability?
   Would a lambda rather than `std::bind` read easier?
   
   Without the side by side diff I honestly can't say I understood what is 
going on here without unwinding all the algorithm magic going on here.




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

To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org

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


Reply via email to