[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings

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

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



##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -69,6 +67,10 @@ add_clangformat(unit_test_callbacks)
 
 enable_testing()
 
+if(UNIX AND NOT APPLE)

Review comment:
       This seems a little wonky... Is this perhaps a Linux or GCC specific dependency? It is also likely that our dependency or `rt` comes from another library and if so that is where this dependency should be added. Also, see root CMakeLists.txt for some of these more platform specific dependencies.

##########
File path: cppcache/CMakeLists.txt
##########
@@ -89,7 +89,6 @@ elseif (WIN32)
 endif()
 
 target_link_libraries(_apache-geode INTERFACE
-  ACE::ACE

Review comment:
       Tears of joy!

##########
File path: cppcache/integration-test/fw_helper.hpp
##########
@@ -107,7 +105,7 @@ BEGIN_TEST.
 #define ASSERT_EQ(x, y) _ASSERT_EQ(x, y, __LINE__, __FILE__)
 
 #define ASSERT_STREQ(x, y)                                            \
-  ASSERT((strcmp(x, y) == 0),                                         \
+  ASSERT(x == y,                                         \

Review comment:
       I would be a little hesitant to change the expected type of x and y from c-string to `std::string`, or really anything with `==` operator overload.
   
   What about `std::strcmp`?

##########
File path: cppcache/src/CppCacheLibrary.cpp
##########
@@ -36,18 +33,15 @@ namespace apache {
 namespace geode {
 namespace client {
 
-void CppCacheLibrary::initLib(void) { ACE::init(); }
+void CppCacheLibrary::initLib(void) {}
 
-void CppCacheLibrary::closeLib(void) {
-  // DO NOT CALL ACE::fini() HERE!
-  // Things might be using ace beyond the life of Geode.
-}
+void CppCacheLibrary::closeLib(void) {}
 
 std::string CppCacheLibrary::initProductLibDir() {
   // otherwise... get the DLL path, and work backwards from it.
-  char buffer[PATH_MAX + 1];
+  char buffer[4097];
   buffer[0] = '\0';
-  DllMainGetPath(buffer, PATH_MAX);
+  DllMainGetPath(buffer, 4096);

Review comment:
       use `sizeof(buffer)` to keep them in sync.

##########
File path: cppcache/src/CppCacheLibrary.cpp
##########
@@ -36,18 +33,15 @@ namespace apache {
 namespace geode {
 namespace client {
 
-void CppCacheLibrary::initLib(void) { ACE::init(); }
+void CppCacheLibrary::initLib(void) {}
 
-void CppCacheLibrary::closeLib(void) {
-  // DO NOT CALL ACE::fini() HERE!
-  // Things might be using ace beyond the life of Geode.
-}
+void CppCacheLibrary::closeLib(void) {}
 
 std::string CppCacheLibrary::initProductLibDir() {
   // otherwise... get the DLL path, and work backwards from it.
-  char buffer[PATH_MAX + 1];
+  char buffer[4097];

Review comment:
       How did you arrive at 4097?
   Perhaps refactoring these implementations of `DllMainGetPath` to allocate and return appropriately sized `std::string` values?
   For Windows the old `MAX_PATH`, 260, limit is gone and [`GetModulFileName`](https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea#return-value) will set an error condition if the buffer it was given was too small. We could increase he buffer as needed to get the full path and the initialize a `std::string` from that buffer before deallocating it.
   For POSIX the [`realpath`](https://pubs.opengroup.org/onlinepubs/009696799/functions/realpath.html) method can allocate the buffer for us by using `NULL` for the `resolved_path` parameter, we just need to free it after initializing an `std:string` with it.




-- 
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-unsubscribe@geode.apache.org

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