Danpf/const map stuff

[danpf]

Anyone have any qualms with this?

the biggest problem is we now have a mutable... but i think that's preferable.

     mutable std::set<std::string> decoded_keys_;

someone wanted to pass a decoder as const & and then they couldn't use decode :(

[speleo3]

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;

[danpf]

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.

[speleo3]

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...

[danpf]

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.

[gtauriello]

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.

[danpf]

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.

[speleo3]

no strong opinion. I'm fine with the mutable.