[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator

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

gaussianrecurrence commented on a change in pull request #660:
URL: https://github.com/apache/geode-native/pull/660#discussion_r497865452



##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -60,20 +63,23 @@ class ThinClientLocatorHelper {
       std::vector<std::shared_ptr<ServerLocation> >& servers,
       const std::string& serverGrp);
   int32_t getCurLocatorsNum() {
-    return static_cast<int32_t>(m_locHostPort.size());
+    return static_cast<int32_t>(m_Locators.size());
   }
   GfErrType updateLocators(const std::string& serverGrp = "");
 
  private:
+  std::vector<ServerLocation> getLocators() const;
   Connector* createConnection(Connector*& conn, const char* hostname,
                               int32_t port,
                               std::chrono::microseconds waitSeconds,
                               int32_t maxBuffSizePool = 0);
-  std::mutex m_locatorLock;
-  std::vector<ServerLocation> m_locHostPort;
+
+  /**
+   * Data members
+   */
+  mutable std::mutex m_locatorLock;

Review comment:
       > The `mutable` keyword is usually a "code smell" - is this just needed in order to declare `getLocators` const?
   
   As stated above I don't agree on getLocators no being a const function member.
   With this premise, as the object state is not changed per-se, and you are using a mutex as a tool to guarantee mutual exclusion, it would make sense for the mutex to be mutable, so lock/unlock operations are allows in a const-qualified scope.
   Regarding this very case, I've seen Herb Sutter has a "M&M rule" which basically summarizes to "mutexes are to be mutable", you can see that here: https://herbsutter.com/2013/05/24/gotw-6a-const-correctness-part-1-3/
   Also, I am awaiting on the response for other C++ comitee members opinion on this matter.
   I get your point about the code-smell, but in this case marking it as such, it doesn't feel right




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