pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441799345
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
Just a thought here. Is it necessary to hash like Java client does in
this case? If the `PdxType::hashcode()` is expected to hash similarly to that
of Java version then changes are needed here. If not then I wouldn't try to
implement the `CacheableKey::hashcode` method here and do something more
compatible with C++, like `std::hash` which produces a `size_t`. In fact I
would just create a specialization of `std::hash<PdxType>`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first,
std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
Review comment:
Equality of hashcode does not guarantee equality of the object. You
should just implement `PdxType::operator==`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first,
std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map<std::shared_ptr<PdxType>, int32_t, PdxTypeHashCode,
PdxTypeEqualCmp>
Review comment:
If you implement `std::hash<PdxType>` and `PdxType::operator==` then you
can use the `dereference_hash<std::shared_ptr<PdxType>>` and
`dereference_equal_to<std::shared_ptr< PdxType >>>` here.
----------------------------------------------------------------
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]
