[Rivet] Dangerous casting to FinalState

[Chris Pollard]

Hi Frank,

Your patch indeed prints out a nice compiler error when I try to copy
construct a FinalState from a VetoedFinalState. I have two issues with this
solution, though (apologies if they have been mentioned earlier):

1) We can no longer copy FinalStates at all. Do we really want to prevent
things like

FinalState fs;
FinalState fs1 = fs;

?? Maybe there's a way around this, but I am not clever enough to see it
immediately.

2) If we decide to hide the copy constructor of FinalState to prevent
confusion, then shouldn't we in principal do the same for every base class
in Rivet? I don't like where this is going...

I am starting to think that we should go ahead and finalize the release and
save this issue for the next one. I am not convinced we will come to a
conclusion in the next few days, but please pipe in if you feel otherwise.

Chris

On Thu, Jul 9, 2015 at 9:38 AM, Chris Pollard <cpollard at cern.ch> wrote:

> Hi Frank,
>
> I tried out your test analysis and hepmc file last night after making a
> local change to remainingFinalState(), and I found the same issues as you
> have. Unfortunately.
>
> "Error in MY_RFSTEST2c::init method: No projections registered for
> parent 0x7ffe07be8840"
>
> This only seems to crash when "remainder" is a VetoedFinalState. When it
> is declared a "const VetoedFinalState&", "FinalState", or "const
> FinalState&", the bookkeeping works (although the physics fails in
> analyze() for FinalState).
>
> I am booked for the next few hours but will hopefully be able to take
> another look in the afternoon. Curse c++.
>
> Chris
>
> On Thu, Jul 9, 2015 at 8:49 AM, Frank Siegert <frank.siegert at cern.ch>
> wrote:
>
>> >> We both agree that the best thing to do is to have
>> remainingFinalState()
>> >> return "const VetoedFinalState&", which is the actual type of the
>> underlying
>> >> returned object. Currently it returns "const FinalState&".
>> >
>> > I agree, and this is part of the patch I posted in this thread.
>> >
>> >> This change
>> >> *should* generate compile-time warnings when a user tries something
>> like
>> >>
>> >> FinalState fs = zfinder.remainingFinalState();
>> >>
>> >> because fs is being copy-constructed from a more complex class.
>> @Frank, I
>> >> think this will also make your attached broken example work as
>> expected.
>> >
>> > Is this a compile-time warning or error, and are you sure it will
>> > really complain? I can test it myself later, but maybe you have
>> > already tested it.
>>
>> I just tested compiling my example containing a
>> FinalState remainder = zfinder.remainingFinalState();
>> with the current tip, to which David has pushed those changes already.
>> There is no warning, let alone a fatal error, when compiling with
>> this.
>>
>> So, do we want to go ahead with my proposed changes to protect the
>> user from the slicing mistake? Consider that even experienced Riveters
>> like Holger and Christian have tripped over this!
>>
>> In that case we would need to understand why
>>   VetoedFinalState remainder = zfinder.remainingFinalState();
>>   addProjection(remainder, "RFS");
>> (i.e. making a full copy, not just by reference)
>> makes Rivet's projection book-keeping fall over:
>>
>>   Error in MY_RFSTEST2c::init method: No projections registered for
>> parent 0x7ffe07be8840
>>
>> I am again attaching the patch (updated to remove the changes David
>> has already committed, needs tip), and the problematic use case
>> (MY_RFSTEST2c.cc) and a HepMC file to test this with:
>>
>> $ cd rivet; patch -p1 < hidden-copy.patch; make install
>> $ rivet-buildplugin MY_RFSTEST2c.cc
>> $ rivet --pwd -a MY_RFSTEST2c file.hepmc
>>
>> Cheers,
>> Frank
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.hepforge.org/lists-archive/rivet/attachments/20150709/228344e3/attachment.html>