port LHCb MT developments to Gaudi master

among others we have

• Gaudi::Functional: base classes to write algorithms as pure functions
• AnyDataWrapper/AnyDataHandle: helper to store any type in the Transient Store (DataSvc)
• fixes to DataObjectHandle
• fixes to GaudiHive
• new tests and examples
• modernization (range loops, removed empty destructors, ...)

This is the same as !240 (closed), needed to fix the problem with the changes view (see INC1210087).

@clemenci - my question was - how do I now end up in the proper hooks if I have code that went this way of declaring dependencies and which I haven't cleaned up yet?

you should end up automatically in the proper hook.

With the new implementation, if you have

class MyAlg: ... {
  DataObjectHandle<MyTracks> m_tracks{"path", Gaudi::DataHandle::Reader, this};
  MyAlg(...) {
    declareProperty("Tracks", m_tracks);
  }
};

it will call PropertyHolder::declareProperty(const std::string& name, DataObjectHandleBase& ref).

Note that the main problem, probably, is that now you must instantiate a DataObjectHandle with a pointer to an owner (and that will take care of the registration to the input/output lists.

The next iteration shuold be that DataObjectHandle<MyTracks> m_tracks{"path", Gaudi::DataHandle::Reader, this}; implies a call to declareProperty.

Exactly I need to do things differently. And it would be nice to have a way that the compiler tells you in case you forgot

Currently you'll get a compilation error if you do not construct correctly a DataObjectHandle, so it's OK.

Once declareProperty is implied by the DataObjectHandle constructor, I can have a warning to tell you to remove the explicit call to declareProperty.

Thanks for re-opening. Yes, removing ranges as said in !240 (closed) would be great.

Added 1 commit:

• b8a4ad4f - Revert Range support in AnyDataHandle

Compare with previous version

I removed the support for ranges in AnyDataHandle. The code compile and the tests pass.

ok perfect!

Validation started with lhcb-gaudi-merge#89

Approved this merge request

Status changed to merged

Mentioned in commit dc1b505f

Marco -

I've finally had a chance to test this against Atlas code, as I've spent the last 6 weeks trying to recover from the utter and complete clusterf*ck that was the seemingly benign "const ToolHandle enforcement" merge, and this also breaks Atlas code, as in our AthAlgTool and AthAlgorithm classes, when we override declareProperty for VarHandles (which inherit from DataHandle), we do a this->declareInput(handle), which is now gone.

What are we supposed to do in its place?

Algorithm inherits from IDataHandleHolder which provides declare -- so you should be be able to replace this->declareInput(handle) with this->declare(handle); and behind the scenes this figures out whether the DataHandle passed is either input or output, and it does the right thing accordingly.

Alternatively, we could (should?) move the implied call to declare which is currently in DataObjectHandleBase up one level into DataHandle, and then declare will be called automatically... so you would not have to do anything except remove this->declareInput(handle)...

A long time ago, we decided that we would have a different inheritance structure for reentrant Algs, as detailed in !177 (closed) , so we don't actually want to check the cardinality of the Algs, especially in the AlgResourcePool, but rather check the isReEntrant() method.

That's where we should be going, but that is a quite an invasive change... so until then, this is quick way to support our migration to Gaudi::Functional as we haven't even migrated away from GaudiAlgorithm yet -- which we should do first, but that requires eg. all calls to tool<T> to be replaced by ToolHandle<T> which is on-going, but not yet finished...

@graven : the DataHandle::setOwner method is also gone. How is the Algorithm supposed to set the ownership of the handle now?

at construction time.

so you want every Algorithm that has a DataHandle as a data member to do this:

class MyAlg {
   SG::ReadHandleKey<EventInfo> m_evt;
}

MyAlg::MyAlg: m_evt("McEventInfo",this) { ... }

yes, see eg. GaudiExamples/src/AlgTools/MyGaudiAlgorithm.cpp

yeah, sorry, this breaks all of current Atlas code.

I'm going to re-introduce the setOwner() method.

is it an option to hide it behind eg #ifdef ATLAS to avoid that LHCb developers are tempted to use it? (and maybe you can asymptotically deprecate it in atlas?)

I don't think that's a good idea.

If EVERY DataHandle of an Algorithm has to pass the this pointer into the constructor, then it makes much more sense for the Algorithm itself to take care of that job, and simplify the constructor of the DataHandle.

No, it's an invariant of the DataHandle that it has an owner, thus that ought to be established at construction, i.e. the constructor should construct an object in a valid state. What would/could you do with a DataHandle without owner? (and doing it this way avoids having code in the body of the constructor, and thus allows one to inherit the constructor from the baseclass, and thus have less boiler plate, and giving developers less chance of doing something wrong)

I TOTALLY disagree. There's VASTLY more boilerplate if EVERY DataHandle has to pass the this pointer into the constructor. And there's room for far more error: what if the user decides to pass something else? If the Algorithm sets it, the user isn't involved in the process at all.

see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerc-ctorac40-define-a-constructor-if-a-class-has-an-invariant and http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerc-completeac41-a-constructor-should-create-a-fully-initialized-object and http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerc-defaultac45-dont-define-a-default-constructor-that-only-initializes-data-members-use-in-class-member-initializers-instead and http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerc-inheritingac52-use-inheriting-constructors-to-import-constructors-into-a-derived-class-that-does-not-need-further-explicit-initialization

The only thing that a user can pass as a valid argument is 'this' -- that's the only thing around that matches the signature. And it matches pattern used for Gaudi::Property, so it has the same 'look and feel'.

@graven @leggett - one of the advantages of this approach is the in-class member initializer. So one-line instead of two. That's incredibly useful for understanding the code - all at one place.

If it's required at construction time, why is it an optional argument?

Also, we need to remember that we're not coding in a vacuum. In the end, there are lots of people who are going to use this code, and it's more important for us to make it easy to use, than to follow abstract coding guidelines. Here you're forcing every user to do a lot more work, with more opportunities for error (what if they forget to pass the this?), when instead it can be removed from their hands entirely. If every user has to do tons of extra, identical work, but we have a way of doing this work for them, then we SHOULD make their lives easier.

No problem in making it a required argument - if there is no corner case that would be killed by it. And that would make things entirely fool-proof. Because then I cannot forget anything w/o the compiler complaining.

Don't forget - the existing approach requires the user to call Algorithm::declare... explicitly within the constructor body. So in fact the current situation forces them to do some work - without any possiblity for checking.

Thus we are making the life of people easier in the end.

Good point -- owner should not be an optional argument, that's for historical reasons.. see !258 (closed)