Which code in FairRunAnaProof initalizes it to the real pointer?

Initialisation of event header is in FairRun. And pointer fEvtHeader in FairRunAnaProof is not owned.

This one is private to FairRunAnaProof. So only FairRunAnaProof can initialize it?

Oh, it seems I misunderstood the "initialisation" of the pointer. If it means assigning a valid address to the pointer, ah, yes, only FairRunAnaProof can initialise it:

That's the only place when it's assigned. And in the Init() function the pointer cannot be guaranteed to have a valid memory address. I will make a change about it very soon. (also in other classes).

I really appreciate, that you're making this private.

But maybe let's do things step by step and keep it protected for the time being and fix up all those users that can't handle a unique_ptr? The major one is probably FillEventHeader(). I had #1255 in my private tree anyway and thought it's now a good time for it. It should make this much simpler. So maybe you might want to wait for it getting merged and then rebase?

I would rather like to keep it private. If I make it protected (, which I tried in the very first beginning), lots of places would need to be revised because of of the naming. If other derived class need to use this private pointer, just call the function GetEventHeader()

Hmmm. Good point!

Could you look into replacing many of the fEvtHeader in the derived classes by GetEventHeader(), or local variables that get initialized by GetEventHeader()? I think, that would make the whole thing more consistant.

Looking at FairRunAnaProof.cxx

fEvtHeader = dynamic_cast<FairEventHeader*>(fRootManager->GetObject("EventHeader."));

That's really confusing. Not to mention I still have trouble to understand whether GetObject returns an owning pointer or not. I guess, it's not owning for now. Which means that the delete in FairRun, that we had before, is wrong.

How about changing it to

fEvtHeader = GetEventHeader();

I don't understand why the event header must be obtained from GetObject? Or they are two different event headers? one from FairRun and another from FairRootManager?

And I don't think GetObject() and Register() from FairRootManager have anything about ownership transferring. Please correct me if I'm wrong.

Thinking again about it…

fEvtHeader = dynamic_cast<FairEventHeader*>(fRootManager->GetObject("EventHeader."));

I think, this assumed (in the current version), that GetObject returns a fresh/new object. Because currently the destructor of FairRun deletes it.

So what about replacing this just with SetEventHeader(…)? Then the unique_ptr will do the delete (again).

Yeah, I would also do this. Event header should be directly obtained from FairRun every time for the consistency.

The current status of #1346 consludes that GetObject returns a non-owning pointer. So the current code that just assigns to fEvtHeader (and lets the destructor of FairRun delete it) would be wrong.

I think, we should first resolve this, before going further?

This one is private to FairRunAnaProof. So only FairRunAnaProof can initialize it?

Is this really needed?

I'm very glad you ask about this. :D Yes, somehow a simple alternative like class FairEventHeader; surprisingly doesn't work. That's the reason why it took me so much time.

The reason behind it is ROOT ( of course, like always! ). If you remove ClassDefOverride in the end of the file, class FairEventHeader works. And about why it works without ClassDefOverride, I have no idea.

Hmmm. Good point!

Could you look into replacing many of the fEvtHeader in the derived classes by GetEventHeader(), or local variables that get initialized by GetEventHeader()? I think, that would make the whole thing more consistant.

Looking at FairRunAnaProof.cxx

fEvtHeader = dynamic_cast<FairEventHeader*>(fRootManager->GetObject("EventHeader."));

That's really confusing. Not to mention I still have trouble to understand whether GetObject returns an owning pointer or not. I guess, it's not owning for now. Which means that the delete in FairRun, that we had before, is wrong.

I don't really like that we have to check each instance of ->GetRunId() for fEvtHeader being set…

What do you think about this (you might want to cherry-pick it into your branch?)

BTW, if you have an ORCiD, append it like this
Family, Give [https://orcid.org/0000-XXXX-XXXX-XXXX].

Ok, thanks for the info.

Thinking again about it…

fEvtHeader = dynamic_cast<FairEventHeader*>(fRootManager->GetObject("EventHeader."));

I think, this assumed (in the current version), that GetObject returns a fresh/new object. Because currently the destructor of FairRun deletes it.

So what about replacing this just with SetEventHeader(…)? Then the unique_ptr will do the delete (again).

Having another thought about this as well:

Could you try to not use member variables, but use local variables in the methods that need fEvtHeader?

Like

FairRunAna::DoFoo() {
    auto fEvtHeader = GetEventHeader();
    …
    // code that actually needs fEvtHeader

that way the relevant methods havr access to the current value and not just an old one?

Yes, this is a better way.

Also relevant to this.

Also relevant to this.

We added the GetSink checks on purpose. Can you please keep them?

Sure

TBH (To Be Honest), I don't understand, why you deprecated FillEventHeader() if you then change it here for example to do exactly what the () variant is doing?

I see FillEventHeader() as the "use the main event header from the current run. And you're just replicating that here?

Hi. By doing so, my idea was that the function FillEventHeader takes the event header as a parameter and therefore is independent of the event header intrinsically. It's a very subtle change. IMO, it's more readable and less error-prone. Otherwise, one possibility to make an error is to change the event header inside the FillEventHeader().

But now I think we should get just rid of it. Instead we could add a getter for the fSource and directly fill the event header from the source. I think it's more clearer in this way.

There is another reason if we consider a little bit design. Basically a good design should only allow one way access and therefore one way for the change. Everything else should be closed.

I learned about the Facade pattern from @dennisklein. And yes, I think, FillEventHeader() and GetEvtHeaderRunId() are facades.

And we're not trying to hide anything away. First, these are internal member functions. Not meant for public use (at least not yet). Second, the full source is available anyway.

I tried to improve documentation with #1413.

From my understanding, the Facade design pattern involves with a Facade object that provides a simpler public interface to a complicated underlying system. But here the function FillEventHeader() just provide an additional way to modify its private member variable.

Protected functions are still not private, which means any derived class could expose the functionality to the public.

However the question remains: there are two paths/ways to do one thing. This could further cause additional problems:

1. Confusing to users.
   Which way should I choose?

auto* evtHeader = run->GetEventHeader();
auto* source = run->GetSource();
source->FillEventHeader(evtHeader);

or

run->FillEventHeader();

2. Bug-prone and difficult to maintain.
   Supposing that in the future there is a change for the function FillEventHeader(), the code using the second way could have this new implementation. But other code using the first method still has the old implementation even though these two method are meant to do exactly the same thing.

Protected functions are still not private, which means any derived class could expose the functionality to the public.

Those functions have been declared (via documentation) as internal in #1413. If someone outside FairRoot uses them, they're entirely on their own. If this is actually a concern, we could mark them as private: and use friend to allow all the relevant users inside FairRoot. But TBH I don't like that either: At some point, I like it, if things are private even inside FairRoot.

Point (1) then isn't relevant: Only possibly confusing to FairRoot developers. My take on that one: Use the shorter API, so that the caller is easier to read (which is really an important point in our current codebase).

Point (2) is also only relevant to FairRoot developers. But that conern is a problematic point of the facade pattern at all. So yes, FairRoot devs have to take care.

Same here… I don't understand, why you need to change this? GetEvtHeaderRunId(); seems to have a nice API?

This is the same reason as above.

I think, this one is causing the final segmentation fault, because GetSource might return a nullptr. The old FillEventHeader() catches this (withut giving a warning…)

If we recommend to unconditionally dereference the return value of GetSource(), we should also make sure it never returns a nullptr, right?

The C++ idiomatic way to communicate this would be to return a reference instead of a raw pointer, but FairRoot rarely uses references ..

Perhaps we should add new safer-to-use APIs to support call chains?

FairSource& GetDataSource();
FairSource const& GetDataSource() const;

GetEventHeader()'s implementation currently guarantees not to return a nullptr, but I would still add a comment to the header file documenting this intended post condition that it does not get violated in the future by accident. Additionally, we should add unit tests.

Yes, the common way for a getter function is to return a const ref. Returning a raw pointer requires the caller to constantly check whether it's a nullptr.

We could implement additional getters to return references. But for GetSource(), is it possible to do one of these?

1. Throw an exception std::runtime_error() if the source is nullptr.
2. Use return gsl::not_null<FairSource*>. This will call std::terminate automatically when fSource is a nullptr. By this way we don't need to change the API.

I would love to fix some of the return types of existing member functions, but how can one do that gracefully in C++ (with a deprecation period)?

Assuming we are not going to break API, gsl::not_null is pretty good shot for those member functions returning a pointer.

For reference, I think we need two getters returning const ref and ref:

auto Get_cref() -> const Type&;
auto Get_ref() -> Type&;

I wouldn't say the following is a good choice:

auto Get() const -> const Type&;
auto Get() -> Type&;

because in many cases when the object is not a const, we still need to get a const ref of its member variable for the reading. Some classes like FairRun can never be a const. But getting a const ref of, for example, event header for its reading is still necessary.

Can we please also keep GetEvtHeaderRunId() for the same reason we keep FillEventHeader()?

Sorry, I still don't understand this style.

FairEventHeader has other getters, like GetInputFiledID, GetEventTime, etc. Does that mean we should also define GetEvtHeaderInputFieldID, GetEvtEventTime, etc in FairRun? If so, we should do the same for fSource. And in the end, FairRun could be filled with more than hundreds of getters and setters.

These are only intended for internal use in FairRun{,Ana,Sim} and not for public use. And so we only have "helpers" for exactly the few interactions with FairEventHeader, that happen often enough in FairRun*, that it is worth to have an internal helper that simplifies internal use. And yes, for FairSource or FairSink, we could have those internal helpers, if they simplify things.

At least that was my idea when I introduced some of these.

Sorry, I can't agree that those internal helper functions simplify things. There is nothing to simplify if there are just one or two lines of code. And on the contrary, it makes things more unreadable and less transparent. For example, from the name of FillEventHeader(), it's difficult to say to where this event header is filled and why it's related to a file source. Same goes for GetEvtHeaderRunId(). It means just one line of code evtHeader->GetRunId() and makes the code less transparent.

I think we should try to write a clean code. And a clean code includes no unnecessary interface or internal method. Sorry about wasting the time for this argument. Maybe a third person opinion could help us address this issue more quickly? @dennisklein

Unfortunately, I don't have a good argument to add, which would favor one over the other 🙈

However, I would like to repeat the comment I made earlier. If such a recommendation like GetEventHeader()->GetRunId() (call chain, which unconditionally de-references returned raw pointers) would be added, let's make some effort it is not going to be dangerous code. (I know there is already a good share of unsafe APIs and not-so-safe coding practices in FairRoot/ROOT, but if we never start to address them, the situation will never improve, even though it is only tiny steps)

I have one proposal:

Let's reduce the diff for this PR to the minimum that's absolutely needed to get fEvtHeader made into a private unique_ptr.
Then let's have a distinct PR that "cleans up" the situation. That way the main maintainers (@karabowi, @fuhlig1) can look at it and decide what they like better.