[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

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



##
File path: cppcache/src/ExceptionTypes.cpp
##
@@ -30,8 +30,8 @@ namespace client {
 void setThreadLocalExceptionMessage(std::string exMsg);
 const std::string& getThreadLocalExceptionMessage();
 
-std::map>
+static std::map>

Review comment:
   Can we make this conform to the naming convention. 
   
   Will be nice if some day this could be `constexpr`. 
   https://www.youtube.com/watch?v=INn3xa4pMfg





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

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations

[GitBox]

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



##
File path: cppcache/benchmark/GeodeLoggingBM.cpp
##
@@ -106,13 +106,13 @@ void GeodeLogToFile(benchmark::State& state) {
   }
 }
 
-auto LogStringsToConsole = GeodeLogToConsole;
-auto LogIntsToConsole = GeodeLogToConsole;
-auto LogComboToConsole = GeodeLogToConsole;
+static auto LogStringsToConsole = GeodeLogToConsole;

Review comment:
   `const` also?

##
File path: tests/cpp/security/XmlAuthzCredentialGenerator.hpp
##
@@ -55,7 +55,7 @@ const opCodeList::value_type QArr[] = {OP_QUERY, 
OP_REGISTER_CQ};
 
 const stringList::value_type QRArr[] = {"Portfolios", "Positions"};
 
-const char* PRiUsnm = "%s%d";
+static const char* PRiUsnm = "%s%d";

Review comment:
   `constexpr` work here?

##
File path: cppcache/integration-test/fw_dunit.cpp
##
@@ -71,10 +71,10 @@ using 
apache::geode::client::testframework::BBNamingContextServer;
 #define __DUNIT_NO_MAIN__
 #include "fw_dunit.hpp"
 
-ACE_TCHAR *g_programName = nullptr;
-uint32_t g_coordinatorPid = 0;
+static ACE_TCHAR *g_programName = nullptr;

Review comment:
   Yuck! I attempted to take on this beast and gave up on trying to clean 
up the old tests. Good job!

##
File path: cppcache/test/CacheXmlParserTest.cpp
##
@@ -99,7 +99,7 @@ std::string valid_cache_config_body = R"(
 
 )";
 
-std::string invalid_cache_config_body = R"(
+static std::string invalid_cache_config_body = R"(

Review comment:
   `const` ?

##
File path: cppcache/src/PdxFieldType.cpp
##
@@ -31,7 +31,7 @@ namespace apache {
 namespace geode {
 namespace client {
 
-int32_t fixedTypeSizes[] = {
+static int32_t fixedTypeSizes[] = {

Review comment:
   `const` and use the `const` naming convention 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.

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