Modernize HltDAQ

• prefer Gaudi::Property and direct member initialization
• prefer delegating to base class constructor

@rmatev FYI

I think this is fine to be merged. (another MR will then come on top with the fix to the HltRoutingBitsFilter.RequireMask default.

@graven, @clemenci, is it worth adding a suitable constructor to Gaudi::Property and putting this in the declaration?

yes, and I've suggested it to @clemenci already -- but in quite a few cases where we now have update handlers, it is actually even better to define a dedicated type, and then add toStream and parse functions to deal with them, and do the work of the updateHandler in parse instead. I've done exactly that in one of the merge requests I submitted yesterday or monday, but I cannot find back which one...

Ah, found it: !457 (diffs). I introduced a Gaudi::Property<TCK> and Gaudi::Property<std::map<TCK,std::string>> where TCK is defined in TCK.h

Aside: it would have perhaps been better if TCK had been defined in a namespace, and if instead of needing toStream in Gaudi::Utils and parse in Gaudi::Parsers it would be found by ADL, i.e. toStream and parse would live in the same namespace as the class they work on....

I think we can allow ADL trivially, just by replacing

--- a/GaudiKernel/GaudiKernel/Property.h
+++ b/GaudiKernel/GaudiKernel/Property.h
@@ -156,7 +156,8 @@ namespace Gaudi
         inline TYPE fromString( const std::string& s )
         {
           TYPE tmp;
-          if ( !Gaudi::Parsers::parse( tmp, s ).isSuccess() ) {
+          using Gaudi::Parsers::parse;
+          if ( !parse( tmp, s ).isSuccess() ) {
             throw std::invalid_argument( "cannot parse '" + s + "' to " + System::typeinfoName( typeid( TYPE ) ) );
           }
           return tmp;
@@ -615,7 +616,8 @@ namespace Gaudi
     void toStream( std::ostream& out ) const override
     {
       m_handlers.useReadHandler( *this );
-      Utils::toStream( m_value, out );
+      using Utils::toStream;
+      toStream( m_value, out );
     }
   };
 

@clemenci: what do you think? Should be fully backwards compatible...

Thanks, I also found the TCK specialization. It's quite nice indeed.

I think it make sense. Create a MR if you can, otherwise a JIRA ticket so that I remember to do it.

Done, see gaudi/Gaudi!242 (merged).

I've checked explicitly that if I add a namespace around TCK (and put a using LHCb::Hlt::TCK at the end of TCK.h for backwards compatibility) , and put parse and toStream in that namespace, that gaudi/Gaudi!242 (merged) indeed allows ADL and that this is sufficient to then define Gaudi::Property<TCK> in any bit of code with only a plain include TCK.h -- i.e. things indeed work as expected.

Even better, you do not need an explicit name space, you can just define toStream and parse as friend functions in the class which you want to make into a property!

Mentioned in merge request !463 (merged)

Added 1 commit:

• 4bebcf96 - Modernize HltDAQ

Compare with previous version

Added 1 commit:

• a021f8ed - make gcc4.9 happy

Compare with previous version

Mentioned in commit 125b0e81

Resolved all discussions

Status changed to merged

Mentioned in commit 34d46b8b

Mentioned in commit 05d0260b