[Rivet] Dangerous casting to FinalState

[Andy Buckley]

A late follow-up on this: on the trunk (i.e. default branch) I just
added a *tiny* tweak to declare a private virtual operator=(const
Projection&) method on the Projection base class. It could possibly be
done better than I have, but I believe this should make it impossible to
copy-assign any classes derived from Projection.

This should ban accidental copies by value. Everything in the Rivet
trunk recompiled just fine, but can you test it with your pathological
examples, Frank?

Andy


On 20/05/15 14:19, Frank Siegert wrote:
> Hi David,
> 
> any ideas? Do you need more information from me?
> 
> Cheers,
> Frank
> 
> 
> 
> On 7 May 2015 at 14:18, Frank Siegert <frank.siegert at cern.ch> wrote:
>> Hi David,
>>
>> We want to apply the attached patch, which hides the dangerous slicing
>> constructors for classes derived from FinalState. I then tested
>> various options of initialising/copying projections, all of which do
>> the correct thing now (errors during compilation for slicing
>> constructors, no errors for faithful ones) except for one:
>>
>> ZFinder zfinder(fs, cut, PID::ELECTRON, 65*GeV, 115*GeV, 0.2,
>> ZFinder::CLUSTERNODECAY, ZFinder::TRACK);
>> VetoedFinalState remainder = zfinder.remainingFinalState();
>> addProjection(remainder, "RFS");
>>
>> I would expect that this should work fine, but at runtime I get:
>>   Error in MY_RFSTEST2c::init method: No projections registered for
>> parent 0x7ffe07be8840
>>
>> Attached is the analysis code and a HepMC file with one event to
>> reproduce it quickly after having applied the patch to Rivet:
>>   $ rivet-buildplugin MY_RFSTEST2c
>>   $ rivet --pwd -a MY_RFSTEST2c file.hepmc2g
>>
>> It would be great if you would have an idea what is going wrong with
>> the projection cloning/registration.
>>
>> Cheers,
>> Frank
>>
>>
>> On 7 May 2015 at 10:50, David Grellscheid
>> <david.grellscheid at durham.ac.uk> wrote:
>>> Hi all,
>>>
>>> I'm sorry I haven't followed this from the beginning and I'm confused
>>> about if we're talking about registered projections or locally created
>>> ones, as the required copying behaviour is more tricky in the first
>>> case. Can someone summarise where I can find the current status?
>>>
>>>   - what you see as the problem
>>>   - what you would expect instead
>>>
>>> Thanks,
>>>
>>>   David
>>>
>>> On 04/05/15 17:28, Andy Buckley wrote:
>>>> On 04/05/15 17:04, Frank Siegert wrote:
>>>>> Hi Andy,
>>>>>
>>>>>> Does that problem disappear if you define remainder as a reference,
>>>>>> either as VetoedFinalState& or FinalState&?
>>>>>
>>>>> Yes, even for FinalState& the problem disappears. That's case 2a in my
>>>>> original examples.
>>>>>
>>>>>> It would also be useful if you could use the -l TRACE flag (if I
>>>>>> remember the syntax correctly) when running, and send the resulting log
>>>>>> so we can see if that parent address corresponds to something. Offhand
>>>>>> it looks like a bit of an odd address value: is this the sort of pointer
>>>>>> value that corresponds to a non-heap-allocated local?
>>>>>
>>>>> I have no idea on the pointer value here, but I have put the full log into
>>>>> http://cern.ch/fsiegert/tmp/trace.log
>>>>
>>>> Thanks!
>>>>
>>>>>> When assigning a Projection like this, a copy will be made. Does
>>>>>> VetoedFinalState have a copy assignment operator, copy constructor, etc.
>>>>>> defined as per the Rule of Three
>>>>>> (http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29)
>>>>>> to make this safe? I genuinely can't remember if we do that, since
>>>>>> copying of projections isn't really the idea. If we don't perhaps we
>>>>>> could make it a rule in Projection implementation to always hide (i.e.
>>>>>> make private) the copy constructor and copy assignment operators to
>>>>>> simply ban copy by value and hence force use of references. I forget if
>>>>>> this can be done on the Projection base class or if each new derived
>>>>>> class would need to explicitly re-hide them. Anyway, food for thought
>>>>>> albeit not quite at black belt level ;-)
>>>>>
>>>>> We don't seem to explicitly define any of the three (copy assignment
>>>>> operator, copy constructor, destructor). I guess the copying that you
>>>>> mean stems from the clone() function, which probably is using the
>>>>> constructor from a FinalState instead of a copy constructor? If our
>>>>> projection registration/cloning can't cope with normal C++ copying
>>>>> then we should indeed either fix it or hide the copy
>>>>> constructors/operators. I have to admit that I didn't even realise
>>>>> that we are internally cloning projections upon registration, and
>>>>> whatever magic is connected to that. It's probably nothing
>>>>> super-urgent to fix, since at least it won't give wrong results. But
>>>>> I'd expect the black-belts to understand this much better than I
>>>>> (challenge!).
>>>>
>>>> Hmm, good point about the cloning. I was just thinking of the local copy
>>>> that occurs if the lvalue isn't a reference type in your example. I'll
>>>> have to have a look at the current clone implementations to know how
>>>> safe they are -- in general if we're not defining copy constructor,
>>>> assignment, and destructor then any projection which holds pointer
>>>> members could be dodgy in copying. I think we've covered the destructor
>>>> angle, but not necessarily the copying ones... so we might want to ban
>>>> those, depending on how clone() is currently written. Old rusty memories
>>>> for me...
>>>>
>>>> Andy
>>>>
> _______________________________________________
> Rivet mailing list
> Rivet at projects.hepforge.org
> https://www.hepforge.org/lists/listinfo/rivet
> 


-- 
Dr Andy Buckley, Lecturer / Royal Society University Research Fellow
Particle Physics Expt Group, University of Glasgow