Fix generation of trigger configurations

There is a consistency check in PropertyConfig::initProperties, which attempts to verify that toString / fromString are each others inverse. With Gaudi v27r1 (2016-patches), the check does not seem to work (fromString seems to have no effect on the property). (edit: the check does seem to be working, but the update handler does not seem to be passed to the clone ) . With Gaudi v28r0, this check is actually working and fromString triggers a call to the property's update handler.

The latter is a problem for LoKi filters, because it triggers the functor decoding, which can't succeed at this stage of execution (e.g. see this nighly):

HltGenConfig                              ERROR failure in update handler of 'Preambulo': HltPV3D:: Unable to decode the functor!

My solution is to remove the update handler from the cloned property before doing the check. This will potentially miss bad behaviour in the update handler, like modifying the property value.

@graven, @raaij, this is a one line fix, but I'm not completely sure it's the proper thing to do. Can you please have a look?

Interesting -- your fix seems to indicate that (now?) update handlers are copied on clone, which presumably was not the case before (? @clemenci) . This raises an interesting question: should they? Because the handler typically has a pointer to an owner by value, and thus typically the owner of the original property gets notified because the clone is modified... In your fix, you disconnect the callback, which indeed fixes the symptom here, but maybe I now understand why the callback has a Property& argument, as then the code can check whether it is invoked with the 'original' property, eg.

diff --git a/Phys/LoKiCore/src/FilterAlg.cpp b/Phys/LoKiCore/src/FilterAlg.cpp
index 9176e90..4447888 100644
--- a/Phys/LoKiCore/src/FilterAlg.cpp
+++ b/Phys/LoKiCore/src/FilterAlg.cpp
@@ -94,8 +94,9 @@ void LoKi::FilterAlg::setPreambulo
 // ============================================================================
 // update the factory 
 // ============================================================================
-void LoKi::FilterAlg::updateFactory ( Property& /* p */ ) // update the factory 
+void LoKi::FilterAlg::updateFactory ( Property&  p  ) // update the factory
 {
+  if (&p != &m_factory) return;
   // no action if not yet initialized 
   if ( Gaudi::StateMachine::INITIALIZED > FSMState() ) { return ; }
   //

and similar in many other places...

And looking at the above diff, maybe it should even be:

--- a/Phys/LoKiCore/src/FilterAlg.cpp
+++ b/Phys/LoKiCore/src/FilterAlg.cpp
@@ -94,10 +94,11 @@ void LoKi::FilterAlg::setPreambulo
 // ============================================================================
 // update the factory 
 // ============================================================================
-void LoKi::FilterAlg::updateFactory ( Property& /* p */ ) // update the factory 
+void LoKi::FilterAlg::updateFactory ( Property&  p  ) // update the factory
 {
+  if (&p != &m_factory) return;
   // no action if not yet initialized 
-  if ( Gaudi::StateMachine::INITIALIZED > FSMState() ) { return ; }
+  if ( Gaudi::StateMachine::INITIALIZED > targetFSMState() ) { return ; }
   //
   // mark as "to-be-updated" 
   m_factory_updated = true ;

i.e. replace FSMState() with targetFSMState() as why update the factory (and other similar things) while transitioning to 'finalized'... But then this shows I think another change might be needed:

--- a/Phys/LoKiCore/src/FilterAlg.cpp
+++ b/Phys/LoKiCore/src/FilterAlg.cpp
@@ -94,13 +94,14 @@ void LoKi::FilterAlg::setPreambulo
 // ============================================================================
 // update the factory 
 // ============================================================================
-void LoKi::FilterAlg::updateFactory ( Property& /* p */ ) // update the factory 
+void LoKi::FilterAlg::updateFactory ( Property&  p  ) // update the factory
 {
-  // no action if not yet initialized 
-  if ( Gaudi::StateMachine::INITIALIZED > FSMState() ) { return ; }
+  if (&p != &m_factory) return;
   //
   // mark as "to-be-updated" 
   m_factory_updated = true ;
+  // no action if not yet initialized
+  if ( Gaudi::StateMachine::INITIALIZED > targetFSMState() ) { return ; }
   //
   // postpone the action 
   if ( !m_code_updated || !m_preambulo_updated ) { return ; } 

i.e. even if not yet in initializing, m_factor_updated should still be set to true unconditionally. This has no effect during the normal order of transitions, as then the result of m_factory is not yet used, and will be used later, but now it does become relevant I think.

Hi,

you are right the new Gaudi::Property copy constructor copies the update handlers. I didn't notice it was not the case before.

I cannot say if it's correct or not, but most probably it's better to keep the old behaviour, also because you demonstrated that adapting the handlers we have it's a nightmare.

OTOH, I would prefer if your check could be avoided, or at least would not require a clone... would you trust a Gaudi::Property<T>::checkStringConversion method?

Another small comment. Gaudi::Property<>::fromString() always returns StatusCode::SUCCESS, or throws std::invalid_argument in case of error.

Cheers Marco

Anything that explicitly test that a Property can do a 'round trip' to a string and back without loss of information would be better ;-) (note: one reason this check is there is because there was a change in Gaudi some time back in 2010 (2011?) or so that changed the string rep of a floating point number by using 'std::fixed' and then setting numbers smaller than the # of digits printed to zero -- and since this reminded my I just checked whether there is any code that forces std::fixed, and yes, there is:

diff --git a/GaudiKernel/src/Util/genconf.cpp b/GaudiKernel/src/Util/genconf.cpp
index 0a43edc..f2cc172 100644
--- a/GaudiKernel/src/Util/genconf.cpp
+++ b/GaudiKernel/src/Util/genconf.cpp
@@ -780,7 +780,7 @@ void configGenerator::pythonizeValue( const PropertyBase* p, string& pvalue, str
     ptype  = "DataObjectHandleBase";
   } else {
     std::ostringstream v_str;
-    v_str.setf( std::ios::fixed ); // to correctly display floats
+    v_str.setf( std::ios::showpoint ); // to correctly display floats
     p->toStream( v_str );
     pvalue = v_str.str();
     ptype  = "list";

I'll make a merge request in Gaudi for the above.

Then on a second note, with gaudi/Gaudi!242 (merged) adding a toStream and parse because much easier, and a large fraction of the uses of updateHandlers can be migrated to use that capability instead. (unfortunately this case isn't one of them I think)

@clemenci, The Gaudi::Property<T>::checkStringConversion() you are talking about will not call the updateHandler, will it? Are you thinking of adding it in v28r1?

I'm just saying that you need to do:

Property<T> a, b;
// ...
b.fromString(a.toString());
assert(a.value() == b.value());

without knowing T at compile time, and this is much easier to implement inside Property (so no need to clone).

The best would be to implement it as

T value;
Gaudi::Details::Property::StringConverter<T> cnv;
assert(cnv.fromString(cvn.toString(value)) == value);

but you need to trust that the implementation of Property::fromString will not cheat.

If you want I'll try to add it for v28r1.

What is the consensus about this MR? Should I merge it?

Since the proposed change replicates the behaviour with Gaudi v27r1, I'd propose to

• merge it
• track the implementation of Marco's proposal and the corresponding change here in JIRA

OK, I'll merge. @rmatev can you open the Gaudi JIRA task?

Status changed to merged

Mentioned in commit 0773f37a

Done, LBHLT-109