[GitHub] [geode-native] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r496186063
##
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:
Got it! I'll have some tests run again just to make sure it is still
green
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] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on a change in pull request #645: URL: https://github.com/apache/geode-native/pull/645#discussion_r488063324 ## File path: cppcache/benchmark/GeodeLoggingBM.cpp ## @@ -35,9 +35,9 @@ using apache::geode::client::internal::geode_hash; const int STRING_ARRAY_LENGTH = 3; -int g_iteration = 0; +static int g_iteration = 0; Review comment: I'll have to look closer at the benchmark code (we have no instructions in the CONTRIBUTING.md file about running benchmarks which seems bad...) For the old style integration tests, I don't think I'd worry about the paradigm, since that code is not really getting updated, more just ported to the new style tests. In the actual production code, I don't think we want to use an anonymous namespace 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] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on a change in pull request #645:
URL: https://github.com/apache/geode-native/pull/645#discussion_r481562738
##
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:
It did not. `ISO C++11 does not allow conversion from string literal to
'char *const'` is the specific error when trying to use it while on a mac.
Haven't looked more into at the moment
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] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on a change in pull request #645: URL: https://github.com/apache/geode-native/pull/645#discussion_r481420359 ## 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: I think the worst part is since I was letting the compiler tell me what was wrong rather than proactively look for places, it built only a partial percent at a time when it was building the integration-test. Thanks for the reminder on places that should be able to use `const`! Putting a link to the [`const` naming style guide](https://google.github.io/styleguide/cppguide#Constant_Names) so I don't forget 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] moleske commented on a change in pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on a change in pull request #645: URL: https://github.com/apache/geode-native/pull/645#discussion_r481420359 ## 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: I think the worst part is since I was letting the compiler tell me what was wrong rather than proactively look for places, I build only a partial percent at a time when it was building the integration-test. Thanks for the reminder on places that should be able to use `const`! Putting a link to the [`const` naming style guide](https://google.github.io/styleguide/cppguide#Constant_Names) so I don't forget 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
