search

rnc-lbl / auau200GeVRun14

LBNL PicoDst HF Analysis Library (PicoHFLib) - Run14 Au+Au 200GeV
1 stars 15 forks source link

StHFMaker: public inheritance and false attempt to force inherited methods to be private #45

Closed MustafaMustafa closed 9 years ago

MustafaMustafa commented 9 years ago

@jthaeder , @mlomnitz

https://github.com/rnc-lbl/auau200GeVRun14/blob/master/StRoot/StPicoHFMaker/StPicoHFMaker.h#L121-L127

I am not sure if this was intended. You are doing a public inheritance from StMaker, what does it mean to override inherited methods as private?! This stuff doesn't work in C++, overriding members of base class does not change the access.

Also, the comment is confusing because this trick won't disallow derived classes from overloading those methods. Beside, "OVERWRITTEN" is the wrong terminology.

I suggest to move them to be public now. Once C++11 is allowed in STAR you can declare these methods as final override.

jthaeder commented 9 years ago

Hi Mustafa,

We discussed this already ;) at the very beginning. And agreed on it 😉

What I do is not forbidden by c++ and obviously works.

Yes the derived class can in principle overwrite that method again.

I just try to visually hide them. Sure somebody can override the methods, but then the code does not work anymore.

Overloaded - overwritten .... Sure, not 100% correct, but I guess that is still understandable. I'm not concerned ...

Cheers Jochen

On May 17, 2015, at 12:42, Mustafa Mustafa notifications@github.com wrote:

@jthaeder , @mlomnitz

https://github.com/rnc-lbl/auau200GeVRun14/blob/master/StRoot/StPicoHFMaker/StPicoHFMaker.h#L121-L127

I am not sure if this was intended. You are doing a public inheritance from StMaker, what does it mean to override inherited methods as private?! This stuff doesn't work in C++, overriding members of base class does not change the access.

Also, the comment is confusing because this trick won't disallow derived classes from overloading those methods. Beside, "OVERWRITTEN" is the wrong terminology.

— Reply to this email directly or view it on GitHub.

jthaeder commented 9 years ago

Actually the label of this issue is wrong. It is not a false attempt .

It is all fine

On May 17, 2015, at 12:41, Mustafa Mustafa notifications@github.com wrote:

Assigned #45 to @jthaeder.

— Reply to this email directly or view it on GitHub.

MustafaMustafa commented 9 years ago

Making the inherited methods private doesn't achieve anything. It is at least obfuscatory, the written code purports something different from what the compiler does. You can check for yourself here. http://ideone.com/8LtYt6

I don't know what "visually hide" means.

P.S. Yes, I remember we discussed it and you said the ALICE something does this, I wasn't convinced much. In any case, I will never make the code convey a message that is different from what the compiler does, this is dangerous.

MustafaMustafa commented 9 years ago

Changed this to a critical question not a bug.

jthaeder commented 9 years ago

Would you be ok with making the inheritance private? Then it should be doing exactly what is expected. (I have to look in the base class first to make sure)

MustafaMustafa commented 9 years ago

Private inheritance is private; if you do it you will not be able to point to StHFMaker with a pointer to StMaker, i.e. your maker will no longer be maker.

My suggestion is to move them to public and that is it. Making them private as they are now has no effect other than being confusing.

jthaeder commented 9 years ago

Made methods public in https://github.com/rnc-lbl/auau200GeVRun14/pull/60 -> declare as final with C++11