[Rivet] Dangerous casting to FinalState

[Andy Buckley]

Hi Frank,

Does that problem disappear if you define remainder as a reference,
either as VetoedFinalState& or FinalState&?

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?

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 ;-)

Andy


On 02/05/15 09:52, Frank Siegert wrote:
> I have adapted my patch to use the more elegant solution of the second reply of
> http://stackoverflow.com/questions/580375/how-to-generate-a-compiler-warning-error-when-object-sliced
> 
> So this solves my issue (i), but the other one still remains, and I
> have to admit that I don't understand why. To remind you:
> 
> (ii) The following valid code:
> VetoedFinalState remainder = zfinder.remainingFinalState();
>       addProjection(remainder, "RFS");
> compiles fine, but at runtime fails with:
>   Error in MY_RFSTEST2c::init method: No projections registered for
> parent 0x7fff33dc1630
> 
> If one of the mentioned black-belts could take a look that would be
> much appreciated. Apart from this one issue I would be ready to commit
> this patch.
> 
> Cheers,
> Frank
> 
> 
> On 1 May 2015 at 11:55, Andy Buckley <andy.buckley at cern.ch> wrote:
>> Any chance we can make progress on this and get it into version 2.2.2?
>>
>> I have to admit I've not ha a chance to think about the technical
>> details or look at the examples. I guess I can do so, but would prefer
>> to spread the technical load a bit... Holger had suggested that he could
>> try to work on it (when not in Sherpa release / ATLAS analysis hell),
>> but maybe some of the C++ black-belts on this list can also take a look.
>>
>> Andy
>>
>>
>> On 24/03/15 16:50, Frank Siegert wrote:
>>> Thanks David for the feedback.
>>>
>>> I'm hoping to get more feedback from the other Riveters as well. Just
>>> to recap, there are two issues with my patch:
>>>
>>> (i) It's a bit ugly to have to hide this for each derived
>>> FinalState explicitly, I don't know whether there is any better way to
>>> disallow such "lossy" copying altogether for a base class?
>>>
>>> (ii) My example 2c is a bit strange... it compiles fine, but at
>>> runtime fails with:
>>> Error in MY_RFSTEST2c::init method: No projections registered for
>>> parent 0x7fff33dc1630
>>>
>>> I'm attaching again the three files from back then in case somebody
>>> doesn't find them anymore.
>>>
>>> Cheers,
>>> Frank
>>>
>>>
>>>
>>> On 14 March 2015 at 20:45, David Bjergaard <david.bjergaard at gmail.com> wrote:
>>>> Ah,
>>>>
>>>> Sorry I forgot to "make install" after applying the patch. I can at
>>>> least confirm the crash:
>>>>> rivet --pwd -a MY_RFSTEST2c zee.hepmc
>>>>> Rivet trunk running on machine calypso (x86_64)
>>>>> Rivet.Analysis.Handler: WARN  Analysis 'MY_RFSTEST2c' is unvalidated: be careful, it may be broken!
>>>>> Error in MY_RFSTEST2c::init method: No projections registered for parent 0x7fff6d42d9a0
>>>>
>>>> David Bjergaard <david.bjergaard at gmail.com> writes:
>>>>
>>>>> Hi Frank,
>>>>>
>>>>> For some reason your test works (with the patch):
>>>>>> rivet --pwd -a MY_RFSTEST2c zee.hepmc
>>>>>> Rivet trunk running on machine calypso (x86_64)
>>>>>> Rivet.Analysis.Handler: WARN  Analysis 'MY_RFSTEST2c' is unvalidated: be careful, it may be broken!
>>>>>> Reading events from 'zee.hepmc'
>>>>>> zfinder: -11 10022
>>>>>> zfinder: 11 10021
>>>>>> rfs.size = 8
>>>>>>   rfs[0] = 2103 10006
>>>>>>   rfs[1] = 2 10007
>>>>>>   rfs[2] = 2103 10009
>>>>>>   rfs[3] = 2 10010
>>>>>>   rfs[4] = 21 10015
>>>>>>   rfs[5] = 21 10016
>>>>>>   rfs[6] = -4 10017
>>>>>>   rfs[7] = 4 10018
>>>>>> Finished event loop
>>>>>> Rivet.Analysis.Handler: INFO  Finalising analyses
>>>>>> Rivet.Analysis.Handler: INFO  Processed 1 event
>>>>>
>>>>>> The MCnet usage guidelines apply to Rivet: see http://www.montecarlonet.org/GUIDELINES
>>>>>> Please acknowledge plots made with Rivet analyses, and cite arXiv:1003.0694 (http://arxiv.org/abs/1003.0694)
>>>>>> Cross-section = 1.515878e+03 pb
>>>>> I wonder if its a compiler difference?
>>>>>> g++ --version
>>>>>> g++ (Ubuntu 4.8.2-19ubuntu1) 4.8.2
>>>>>> uname -a
>>>>>> Linux calypso 3.13.0-46-generic #77-Ubuntu SMP Mon Mar 2 18:23:39 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
>>>>>
>>>>> Cheers,
>>>>>
>>>>>     Dave
>>>>>
>>>>> Frank Siegert <frank.siegert at cern.ch> writes:
>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> Thanks for your feedback.
>>>>>>
>>>>>>> Just thinking out loud:
>>>>>>> addProjection is templated on a reference, is it possible to make a
>>>>>>> specialized template for the non-reference version?
>>>>>>>
>>>>>>> Something like:
>>>>>>>     template <typename PROJ>
>>>>>>>     const PROJ& addProjection(const PROJ proj, const std::string& name) {
>>>>>>>       std::cerr<<"Please be careful about using references!"<<std::endl;
>>>>>>>       return proj;
>>>>>>>     }
>>>>>>
>>>>>> But we would not want to prevent *registering* projections by copy,
>>>>>> since that's in general absolutely fine, right? It's rather the
>>>>>> copying of a derived projection, e.g. VetoedFinalState, into a
>>>>>> FinalState that seems dangerous to me.
>>>>>>
>>>>>> I have just tested that it's possible to hide the copy constructor,
>>>>>> and am attaching a patch for Rivet which shows how this would work in
>>>>>> real life. It's a bit ugly to have to hide this for each derived
>>>>>> FinalState explicitly, I don't know whether there is any better way to
>>>>>> disallow such "lossy" copying altogether for a base class?
>>>>>>
>>>>>> With this patch Rivet compiles fine, and a few of the typical ZFinder
>>>>>> analyses seem to work as expected. Furthermore, an accidental VFS ->
>>>>>> FS copying like in my Example 1 now fails as expected and 2a and 2b
>>>>>> work as expected. But 2c is a bit strange... it compiles fine, but at
>>>>>> runtime fails with:
>>>>>> Error in MY_RFSTEST2c::init method: No projections registered for
>>>>>> parent 0x7fff33dc1630
>>>>>>
>>>>>> I don't at all understand where that's coming from, but there is a lot
>>>>>> of casting in the projection registration going on. Any ideas? I'm
>>>>>> attaching a simple analysis that demonstrates this behaviour, together
>>>>>> with one Zee event for testing.
>>>>>>
>>>>>> Cheers,
>>>>>> Frank
>>>>>>


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