Closed danpf closed 5 years ago
I never really saw the need for checkExtraKeys()
, except for debugging and development maybe. So my suggestion would be to make checkExtraKeys()
a no-op if the instance is const
.
diff --git a/include/mmtf/map_decoder.hpp b/include/mmtf/map_decoder.hpp
index ba77c44..0349ead 100644
--- a/include/mmtf/map_decoder.hpp
+++ b/include/mmtf/map_decoder.hpp
@@ -103,10 +103,24 @@ public:
* @brief Check if there are any keys, that were not decoded.
* This is to be called after all expected fields have been decoded.
* A warning is written to stderr for each non-decoded key.
+ *
+ * This is a no-op for const references.
*/
- void checkExtraKeys() const;
+ void checkExtraKeys();
+ void checkExtraKeys() const {};
private:
+
+ /**
+ * Remember that this key has been decoded (for checkExtraKeys).
+ *
+ * This is a no-op for const references.
+ */
+ void decoded_keys_insert(const std::string& key) {
+ decoded_keys_.insert(key);
+ }
+ void decoded_keys_insert(const std::string& key) const {}
+
// when constructed with byte buffer, we keep unpacked object
// NOTE: this contains a unique pointer to msgpack data (cannot copy)
msgpack::object_handle object_handle_;
@@ -114,7 +128,7 @@ private:
typedef std::map<std::string, const msgpack::object*> data_map_type_;
data_map_type_ data_map_;
// set of keys that were successfully decoded
- mutable std::set<std::string> decoded_keys_;
+ std::set<std::string> decoded_keys_;
/**
* @brief Initialize object given an object
@@ -174,7 +188,7 @@ inline MapDecoder::copy_decode(const std::string& key, bool required,
// note: cost of O(M*log(N)) string comparisons (M parsed, N in map)
data_map_type_::const_iterator it = data_map_.find(key);
if (it != data_map_.end()) {
- decoded_keys_.insert(key);
+ decoded_keys_insert(key);
// expensive copy here
msgpack::object tmp_object(*it->second, zone);
tmp_object.convert(target);
@@ -197,7 +211,7 @@ inline void MapDecoder::decode(const std::string& key, bool required, T& target)
} else {
it->second->convert(target);
}
- decoded_keys_.insert(key);
+ decoded_keys_insert(key);
}
else if (required) {
throw DecodeError("MsgPack MAP does not contain required entry "
@@ -205,8 +219,7 @@ inline void MapDecoder::decode(const std::string& key, bool required, T& target)
}
}
-
-inline void MapDecoder::checkExtraKeys() const {
+inline void MapDecoder::checkExtraKeys() {
// note: cost of O(N*log(M))) string comparisons (M parsed, N in map)
// simple set difference algorithm
data_map_type_::const_iterator map_it;
I'm ok either way, but I think it sort of makes it a useless debugging tool if we're going to make it un-useable ~half the time.
I didn't really think this through, sorry. If decode(...)
and copy_decode(...)
are const, then it will always call the const overload of decoded_keys_insert(...)
. So my patch doesn't make sense unless you provide a non-const overload of decode
/copy_decode
as well...
gotcha.... I've never actually used the debugging features of checkExtraKeys so i'd be fine getting rid of it. But i'm hesitant to do that, since someone might actually find it handy.
As @speleo3 correctly pointed out, it was always just meant for debugging and development.
I have no problem with having a mutable private field as you did it.
If you want to get rid of the whole checking-logic for release-builds (or make it clear that this is a debugging code), I would simply allow for it to be disabled with the good-old NDEBUG macro (i.e. put #if !defined(NDEBUG)
& #endif
around it)?
I don't like having different behaviors for const or non-const. That would be confusing...
Like this we are consistent with usual debugging in C++ (i.e. same as the assert
function) and anyone using cmake with release settings (i.e. CMAKE_BUILD_TYPE=Release
) would have all of those checks disabled by default.
I think it's fine the way it is now until someone explicitly wants/needs it gone. It should almost never be triggered in the places we call it now, so it might serve as a warning to someone who's trying to do something too hacky.
no strong opinion. I'm fine with the mutable.
Anyone have any qualms with this?
the biggest problem is we now have a mutable... but i think that's preferable.
someone wanted to pass a decoder as const & and then they couldn't use decode :(