Deprecate Close method in FairSource/Sink

[YanzhaoW]

• Deprecation of Close method in FairSource/Sink.
• Move the Close executions in the derived classes to their dtor (if needed).
• Remove callings of the Close methods in FairRootManager and FairRun.

---

Checklist:

• [x] Followed the Contributing Guidelines

[YanzhaoW]

a sidenote: I have rewritten the FairFileSource class in R3BRoot in this PR (actually it's R3BFileSource, which is a copy-paste from FairFileSource). Maybe we could migrate the new class here?

[karabowi]

I have tested FairRoot's private branch without FairSampler::Close() and FairSink::Close() with PandaRoot. I did not observe any crash.

[ChristianTackeGSI]

Deprecations in General

For me a deprecation means, that

1. All old code stays as it is (as long as it can be considered public API). So we should not change too much code. So that users' code continues to work as without this PR!
2. Any public API that should not be used in the future by users, should be marked as deprecated (either via [[deprecated]], or via comments, if the former is not possible).

In FairRoot: Public API

Let's look at (1):

Public API is basically everything in fairroot/. So we should not change any code or behaviour there, basically. Of course we can do some simple stuff: Instead of calling FairRootManager::CloseSink (which calls fSink->Close();) we can call fSink->Close(); directly. This does not change any actual behaviour.

When it comes to examples/ we should go the full route! We should show to users what we expect their code to look like in the future.

FairRoot: The confusing Close situation

In a cool world, the old situation would look like this:

class FairSourceSink {
public:
    void CloseForUsers() { CloseImpl(); }
private:
    // Please override in your implementation
    virtual void CloseImpl() = 0;
private:
    // So that the library can call private member functions
    friend FairRun;
    friend FairRootManager;
    // Will be called internally by the library as a "call back like thing"
    void CloseForInternalUse() { CloseImpl(); }
};

Let's go step by step.

1. We want to deprecate CloseForUsers, so that users do not call it any more:

   public:
       [[deprecated]] void CloseForUsers() { CloseImpl(); }
   

   Note: CloseForInternalUse is not deprecated. We want to continue calling it from the library, so that the CloseImpl is still called.

   Corollary: We would remove any calls to CloseForUsers from examples/!

2. We want to deprecate overriding CloseImpl:

   private:
       // \deprecated Please stop overriding it!
       virtual void CloseImpl() {}
   

   Note 1: There is no point in marking it override. We want to deprecate overriding it. No need to deprecate calling it. It can only be called by us. All of our business.

   Note 2: We still continue calling CloseForInternalUse (to keep bevahiour exactly as it is)

   Note 3: We would still keep the overriding in fairroot/, because we did not yet remove CloseForUsers(). So code that depends on CloseForUsers() calling something should be happy.

   Corollary: We would remove any overrding of CloseImpl from examples/.

3. Remove CloseForUsers, keep everything else the same

4. Remove CloseImpl, and remove all of CloseForInternalUse.

That's my current idea on how this should look like.

The problem is now, that all of those three (CloseImpl, CloseForInternalUse, and CloseForUsers) are currently in one function Close to make things complex.

To maybe simplify things, we could temporarily invent:

private
   void CloseForInternalUse() {
       // All of these pragmas, because we would call
       // CloseImpl (which is not deprecated internally),
       // but we have to call Close, which is deprecated
       // from (1).
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
       Close();
#pragma GCC diagnostic pop
   }

That's the whole problem here from my point of view.

[YanzhaoW]

Thanks for the replies.

Sorry I'm very busy working on other things and don't have time to deal with this PR these days. But probably on this Friday I will read the replies in details and continue to fix the things here if needed.

[YanzhaoW]

Hi,

I thought it over again. Yes, the procedures described above is probably the right way to go. But there is still something weird that I couldn't pinpoint what exactly it is in the beginning, especially when I see:

private
   void CloseForInternalUse() {
       // All of these pragmas, because we would call
       // CloseImpl (which is not deprecated internally),
       // but we have to call Close, which is deprecated
       // from (1).
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
       Close();
#pragma GCC diagnostic pop
   }

For me, we are kind of contradicting ourself when we enable a warning from a new deprecation but at the same time we also disable it. I guess the people who designed the "deprecation" in C++ also didn't expect the people to disable it at the same time using a compiler-specific macro (There are some FairRoot users using apple-clang).

Our main dispute is whether we should remove the calling of the FairSink::Close() or keep it in the public API of FairRootManager:

void CloseSink()
{
    if (fSink) {
        fSink->Close();
    }
}

Here are my arguments why it should just be removed based on the following principles:

1. Deprecation should only be applied to public API.
2. Public API shouldn't be called internally.
3. Virtual functions should be private and shouldn't be called in the derived classes.

The point 2 and 3 are supported by C++ guidelines from Herb Sutter about why virtual functions should be private. In this articles, it says

Prefer to use Template Method to make the interface stable and nonvirtual, while delegating customizable work to nonpublic virtual functions that are responsible for implementing the customizable behavior. After all, virtual functions are designed to let derived classes customize behavior; it's better to not let publicly derived classes also customize the inherited interface, which is supposed to be consistent.

It also implies here that the derived classes which overrides the virtual functions are just customization. For example, if we are designing an abstract class for a person wearing a T-shirt and pants. It's the job of the derived classes (User's classes) to decide what kind of T-shirt and pants are (e.g colors and sizes). But the logic connection between those objects are determined by the abstract class (the library implementer). Users shouldn't be concerned about in which order they are put up or whether they are put up or not. Suppose if we want to let the person wear T-shirt first instead of pants first, we could just change that order without doing any deprecation of the old order. Deprecation is used to notify the users. If it's irrelevant to users, it should not be deprecated but rather simply removed.

So now let's get back to CLoseSink(). Here the closing of the fSink is an internal logic for FairRootManager. Users of FairRootManager only interact with the public API CloseSink instead of fSink directly. For them, what's inside this public API shouldn't be their concerns and shouldn't break their code if changed. So if we need to discard the Close() method of FairSink, I think it's totally fine to be removed in FairRootManager::CloseSink() now.