opened
mwoehlke-kitware
username_0
Add a new method to `metadata` to retrieve an item if present, or clearly indicate (by returning a null pointer) if no such item is present. This saves having to perform two lookups to avoid the "undefined result" of the old look-up method. Also update users to use the new method, and remove the no-longer-needed `find`.
 Improve API for metadata collections
issue
created
linus-sherrill
username_1
I agree with finding a way around querying the map twice, but my concern with returning a null_ptr, is that forgetting to check the return value is a very common mistake. I am not sure what the alternative would be? optional<>?
comment
created
mwoehlke-kitware
username_0
Yeah, I think that would work (I think `optional<T&>` does not add overhead compared to `T*` provided you use its `operator*` rather than `value`), but alas, that requires C++17... unless we want to roll our own version until then? (It would be easy to make one that only holds references... IIRC all the tricky bits have to do with holding *values* and exception safety.)
comment
created
mleotta
username_2
One option here is to keep the `get` function, but have it return a reference as before and go back to returning the special unknown metadata object when a value is not found.  To avoid the use of `has` and the double search we could add a `virtual bool is_valid();` member function that returns true for all real metadata items and return false for the special unknown metadata object.  For convenience you could then define a boolean cast function for the metadata item that calls this `is_valid` function.  So you could then use the metadata item much like we use the pointer in this branch with
```c++
if(auto const& item = md.get(NAME))
{
  double val = item.as_double();
}
```
comment
created
linus-sherrill
username_1
I like the behaviour of Matt's suggestion. The unknown metadata object could then throw something. This is better than a seg-fault.
comment
created
mwoehlke-kitware
username_0
The down side of @username_2's suggestion (and we had previously discussed exactly this option) is that it adds the overhead of a *virtual* call to just about every user. This isn't Java/Python; we use pointers (often smart pointers, but those still don't protect you against dereferencing a null) all over the place. *If* we were to do that, I would prefer to do it by re-adding / un-removing `find` with the safer behavior. "Don't pay for what you don't need."
comment
created
mleotta
username_2
I think the expense of a virtual function call to check if an object is valid is negligible here.  I also like the idea @username_1 had of throwing an exception if you try to access values from in valid item.  This allows users to not check if a metadata item is valid (in some cases you can assume that it should always be there) and then catch exceptions in the rare case that something went wrong.

If you are really against the virtual function you could have the unknown item be a static, globally accessible instance and then you could just compare the memory address to see if what you got was the one and only unknown item.  However, I would still go with the virtual function.
comment
created
mwoehlke-kitware
username_0
Jenkins test this please
comment
created
mwoehlke-kitware
username_0
So... in this revision, I've basically left `find` alone (aside from clarifying in the documentation that it *will* return a "usable" object), but added `metadata_item::is_valid`. If you want safety, use `find`. If you want efficiency, use `get`, especially if you are writing something like `if (auto i = md.get(...)) { ...use i... }`. Note that `get` is approximately as safe as `std::map::find`¹, and *exactly* as safe as any of the many, *many* functions in KWIVER that return some flavor of `std::shared_ptr`.

(¹ Both are [UB](https://en.wikipedia.org/wiki/Undefined_behavior) if the item was not found and you try to use it without checking. The latter may crash or may lead to silent memory corruption, whereas `get` almost certainly *will* crash.)
comment
created
mwoehlke-kitware
username_0
Since I am apparently outvoted, this version a) just has `find` and b) requires calling the virtual method `is_valid`¹ to know if the result of `find` is a "real" item or the "fake" 'not found' item.

(¹ ...which will almost always happen implicitly via `operator bool`.)
comment