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:
[email protected]
