You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/02/22 13:02:18 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #928: GEODE-10074: Remove ACE remainings
gaussianrecurrence opened a new pull request #928:
URL: https://github.com/apache/geode-native/pull/928
- Removed all the ACE includes along the repository
- Removed ACE from CMake project.
- Removed ACE dependency.
- There were a couple of files in which ACE's strcmp was used.
So, its usage was replaced to to std::string::operator==
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813263489
##########
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:
Perhaps something more like this?
```
find_library(LIB_RT rt)
if (LIBRT)
target_link_libraries(<target> ${LIB_RT})
endif()
```
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813263489
##########
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:
Perhaps something more like this?
```
find_library(LIB_RT rt)
if (LIBRT)
target_link_libraries(<target> ${LIB_RT})
endif()
```
##########
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:
Perhaps something more like this?
```
find_library(LIB_RT rt)
if (LIBRT)
target_link_libraries(<target> ${LIB_RT})
endif()
```
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813859028
##########
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:
Yes! So that! One less macro!
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r814181420
##########
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:
Oh, well then if it is just these legacy tests... hack away.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813704606
##########
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:
Ups, my bad, I did hardcode this while testing and forgot to properly solve it. Thanks for pointing it out :)
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813702430
##########
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:
It's true that my solution for this is not the best one.
Regarding root CMakeLists.txt I don't think it makes sense, as rt is only needed for the shared memory API, which is used only by integration-tests. I'll try the find_library approach for this
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813706974
##########
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:
Well thing is this ASSERT_STREQ macro is only used in one place within integration-tests, so I'd rather remove this macro and use operator== in-place.
For reference here is the only place where it's used: https://github.com/apache/geode-native/blob/a072c8f69aa59335ab0b134f435f174c97d0ac22/cppcache/integration-test/testSystemProperties.cpp#L61
As you can see it's safe to remove this macro and use operator== from std::string
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by 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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #928: GEODE-10074: Remove ACE remainings
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #928:
URL: https://github.com/apache/geode-native/pull/928#discussion_r813704882
##########
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:
See below comment
--
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