[
https://issues.apache.org/jira/browse/GEODE-8601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17249403#comment-17249403
]
ASF GitHub Bot commented on GEODE-8601:
---------------------------------------
mreddington commented on a change in pull request #678:
URL: https://github.com/apache/geode-native/pull/678#discussion_r542925100
##########
File path: cppcache/include/geode/CacheStatistics.hpp
##########
@@ -97,11 +97,11 @@ class APACHE_GEODE_EXPORT CacheStatistics {
virtual time_point getLastAccessedTime() const;
private:
- virtual void setLastAccessedTime(time_point lat);
- virtual void setLastModifiedTime(time_point lmt);
+ virtual void setLastAccessedTime(const time_point& tp);
Review comment:
A time_point boils down to maybe a `long long`, is there any pressing
reason to pass by reference, and thus cascade a const reference type change to
this interface through all the derived implementations in all client code?
##########
File path: cppcache/include/geode/CacheStatistics.hpp
##########
@@ -47,9 +47,9 @@ class LocalRegion;
*/
class APACHE_GEODE_EXPORT CacheStatistics {
public:
- typedef std::chrono::system_clock::time_point time_point;
+ using time_point = std::chrono::steady_clock::time_point;
- CacheStatistics() : m_lastAccessTime(0), m_lastModifiedTime(0) {}
+ CacheStatistics() = default;
Review comment:
`default` will produce an inlined definition that will be compiled in
client code for library code. This is just asking for type pruning problems,
which we've seen before. This constructor should instead be moved into the
source file. Default is fine for internal implementation, not for published
headers.
##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -114,6 +115,7 @@ endif()
target_link_libraries(${TEST}
PRIVATE
ACE::ACE
+ Boost::boost
Review comment:
Same.
##########
File path: cppcache/include/geode/CacheStatistics.hpp
##########
@@ -97,11 +97,11 @@ class APACHE_GEODE_EXPORT CacheStatistics {
virtual time_point getLastAccessedTime() const;
private:
- virtual void setLastAccessedTime(time_point lat);
- virtual void setLastModifiedTime(time_point lmt);
+ virtual void setLastAccessedTime(const time_point& tp);
+ virtual void setLastModifiedTime(const time_point& tp);
- std::atomic<time_point::duration::rep> m_lastAccessTime;
- std::atomic<time_point::duration::rep> m_lastModifiedTime;
+ std::atomic<time_point::duration::rep> last_accessed_{0};
Review comment:
This change also inlines into client code. Don't get me wrong, the
original constructor was just as wrong, it should have just been moved into the
source file. Save this sort of thing for internal implementation, not published
headers.
##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -25,6 +25,7 @@ add_library(test-cppcache-utils STATIC
target_link_libraries(test-cppcache-utils
PRIVATE
ACE::ACE
+ Boost::boost
Review comment:
Do you need all of Boost? You should be able to include just the
libraries you need, Boost::asio, for example.
----------------------------------------------------------------
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
> Replace ACE ExpiryTaskManager impl by boost::asio one
> -----------------------------------------------------
>
> Key: GEODE-8601
> URL: https://issues.apache.org/jira/browse/GEODE-8601
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Mario Salazar de Torres
> Assignee: Mario Salazar de Torres
> Priority: Major
> Labels: obliterate-ace, pull-request-available
>
> *AS A* native client contributor
> *I WOULD LIKE* to replace ACE implementation of ExpiryTaskManager for a
> boost::asio one
> *SO THAT* all issues related to it disappear, the implementation is more
> simple and move away from ACE as it's the project policy for a while.
> —
> *Additional information*
> Current implementation of ExpiryTaskManager is causing issue GEODE-8535 and
> some other related, not yet reported.
> Also, as I am aware the project is moving away from ACE, so I'd say this is
> the perfect oportunity to replace ExpiryTaskManager implementation.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
