pivotal-jbarrett commented on a change in pull request #618:
URL: https://github.com/apache/geode-native/pull/618#discussion_r439207032
##########
File path: cppcache/src/MapSegment.cpp
##########
@@ -731,7 +731,7 @@ GfErrType
MapSegment::isTombstone(std::shared_ptr<CacheableKey> key,
bool& result) {
std::shared_ptr<Cacheable> value;
std::shared_ptr<MapEntryImpl> mePtr;
- const auto& find = m_map->find(key);
+ auto find = m_map->find(key);
Review comment:
Rather than incurring the copy cost here just scope this variable with
some bracing. Or use different variable names for each `find` result. If they
are immutable the compiler has a better chance optimizing them away.
##########
File path: cppcache/src/LRUList.cpp
##########
@@ -102,27 +102,25 @@ void LRUList<TEntry, TCreateEntry>::getLRUEntry(
template <typename TEntry, typename TCreateEntry>
typename LRUList<TEntry, TCreateEntry>::LRUListNode*
LRUList<TEntry, TCreateEntry>::getHeadNode(bool& isLast) {
- std::lock_guard<spinlock_mutex> lk(m_headLock);
+ std::lock_guard<spinlock_mutex> headSpinlock(m_headLock);
LRUListNode* result = m_headNode;
LRUListNode* nextNode;
- {
- std::lock_guard<spinlock_mutex> lk(m_tailLock);
-
- nextNode = m_headNode->getNextLRUListNode();
- if (nextNode == nullptr) {
- // last one in the list...
- isLast = true;
- std::shared_ptr<TEntry> entry;
- result->getEntry(entry);
- if (entry->getLRUProperties().testEvicted()) {
- // list is empty.
- return nullptr;
- } else {
- entry->getLRUProperties().setEvicted();
- return result;
- }
+ std::lock_guard<spinlock_mutex> tailSpinlock(m_tailLock);
Review comment:
The scope of the `m_tailLock` has been extended to the of of this method
along with the `m_headLock`.
##########
File path: cppcache/src/ClientMetadataService.cpp
##########
@@ -765,18 +766,17 @@ void
ClientMetadataService::markPrimaryBucketForTimeoutButLookSecondaryBucket(
serverLocation, version);
std::shared_ptr<ClientMetadata> cptr = nullptr;
- {
- boost::shared_lock<decltype(m_regionMetadataLock)> lock(
- m_regionMetadataLock);
- const auto& cptrIter = m_regionMetaDataMap.find(region->getFullPath());
- if (cptrIter != m_regionMetaDataMap.end()) {
- cptr = cptrIter->second;
- }
+ boost::shared_lock<decltype(m_regionMetadataLock)> boostRegionMetadataLock(
Review comment:
You have changed the scope of this lock by removing the block
surrounding it. The lock is now held until the end of this method rather than
being released after line 780.
##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -439,7 +440,7 @@ GfErrType TcrEndpoint::registerDM(bool clientNotification,
bool isSecondary,
"channel for endpoint %s",
m_name.c_str());
// setup notification channel for the first region
- std::lock_guard<decltype(m_notifyReceiverLock)> guard(
+ std::lock_guard<decltype(m_notifyReceiverLock)> notifyReceiverLockGuard(
Review comment:
Oh wow, that was a bad one. Confusing to rationalize when `guard` would
be released.
##########
File path: cppcache/src/Log.cpp
##########
@@ -257,7 +257,7 @@ void Log::init(LogLevel level, const char* logFileName,
int32_t logFileLimit,
if (status != -1) {
for (int index = 0; index < sds.length(); ++index) {
std::string strname = ACE::basename(sds[index]->d_name);
- size_t fileExtPos = strname.find_last_of('.', strname.length());
+ fileExtPos = strname.find_last_of('.', strname.length());
Review comment:
Change previous declaration to `auto`.
##########
File path: cppcache/src/Log.cpp
##########
@@ -280,7 +280,7 @@ void Log::init(LogLevel level, const char* logFileName,
int32_t logFileLimit,
std::string extName;
std::string newfilestr;
- int32_t len = static_cast<int32_t>(g_logFileWithExt->length());
+ len = static_cast<int32_t>(g_logFileWithExt->length());
Review comment:
Change previous declaration to `auto`.
##########
File path: cppcache/src/Log.cpp
##########
@@ -657,7 +657,7 @@ void Log::put(LogLevel level, const char* msg) {
char printmsg[256];
std::snprintf(printmsg, 256, "%s\t%s\n", "Could not delete",
fileInfo[fileIndex].first.c_str());
- int numChars =
+ numChars =
Review comment:
Change previous declaration to `auto`.
----------------------------------------------------------------
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:
us...@infra.apache.org
