Misbehavior of ParameterSet::put(...) and insert(...) functions when key exists
If the specified key for an insert or put exists, the operation will silently fail. It is also seriously non-trivial to ascertain whether a parameter exists (
ParameterSet::get_keys() followed by
put() should replace any existing value. However, I believe it is also desirable to provide a
bool has_key() method to allow the user to check for presence first.
#2 Updated by Rob Kutschke over 7 years ago
Here is my two cents...
I like the addition of a "hasName" method.
I prefer that put/insert have an optional, argument that specifies what to do if the name already exists: the two options should be to replace the existing definition or throw an exception. I prefer that the default be to throw an exception. We could also consider a third option: to allow replacement if and only if the "type" of the old and new values match; that is, we can replace a sequence with another sequence but not with a table or an atomic value.
Perhaps it also makes sense to have a "replace" method that just forwards to "put" but which sets the optional argument to allow replacement.
#5 Updated by Christopher Green almost 6 years ago
- Status changed from Accepted to Feedback
- Assignee set to Christopher Green
- % Done changed from 0 to 40
has_key() was added with fhicl-cpp:7ff98d3fdba1c9ea319eff5aa7fdd45c1eaef2cc.
We propose that
insert() be taken private, because the ability to decode the encoded value relies on the correct construction of the
boost::any, which cannot be left safely to the non-expert.
We further propose that
put() throw if insert is not achieved, and that we provide the alternative function
put_or_replace with the replace-if-existing behavior based on key only. Type checking is, unfortunately, not practical in the general case.
Does this sound reasonable?
#6 Updated by Rob Kutschke almost 6 years ago
Please go ahead with the proposal.
I don't want to give up on the limited notion of type checking that we asked for but it's important to get the agreed upon functionality out the door. Should I open a new issue for the stuff we would still like - or is it so hard that it is unrealistic to ever expect it?
#7 Updated by Christopher Green almost 6 years ago
The problem is that the only way to test whether a parameter is of a particular C++ type (or compatible with same) after encoding is to attempt the conversion and deal with the exception once thrown. It is generally not advisable to use exceptions as standard flow control as this (among other things) complicates debugging. There are several functions that aid one in distinguishing whether the key represents an atom, sequence or table, but once you have an atom determining its type requires an attempted
any_cast(), because it is stored simply as a string.
#8 Updated by Rob Kutschke almost 6 years ago
Sorry. I was not clear.
I was not requesting anything to do with C++ type. I only meant FHiCL type: if a FHiCL name is already defined as an atom, I would like an option to say that redefining it as a non-atom is an error. ( With the corner case that @nil is no type and can be redefined as anything ). I imagine that I only get this choice once per .fcl file - is that right?
#9 Updated by Christopher Green almost 6 years ago
Ah, I understand now, sorry.For the checking behavior you actually say you want, we would provide a function (say),
put_or_replace_compatible()which would do the necessary checking and throw on an incompatible replace attempt. So, to summarize, the user-visible interface would be:
template< class T > void put (std::string const & key, T const & value);Throw on existing key.
template< class T > void put_or_replace (std::string const & key, T const & value);Generally succeed.
template< class T > void put_or_replace_compatible (std::string const & key, T const & value);Throw if replacing an item with a different major FHiCL type.
Does this seem reasonable?
#10 Updated by Rob Kutschke almost 6 years ago
Yes. That's what I am looking for.
Is it also possible to make it configurable per policy so that the Mu2e executable chooses put_or_replace_compatible when the run time configuration is parsed?
I need to check with Andrei and a few others before I decide that this is what we really want. But I would like to know if it is a realistic request.
#11 Updated by Christopher Green almost 6 years ago
Choosing the behavior in code is easy (whatever function the user decides to call); allowing the coherent behavior to be configured by the application at run time is hard, and not something we've thought about before. I'm loathe to do it the quick and dirty way (a global), but anything else will require thought and design. Can I suggest that we implement the suggestions I made earlier for the three different user-facing functions, and ask that you submit a different feature request for the run time configurability you desire?
#12 Updated by Rob Kutschke almost 6 years ago
I like the proposed features - please get them in now.
As I remember the start of this discussion (maybe not captured well in the ticket) the goal was the catch a class of errors at FHiCL parsing time and for this feature to be available when the main art configuration file was parsed. But we can make this a separate ticket if needed.