[Rivet] Dangerous casting to FinalState

[David Bjergaard]

Hi All,

A humble suggestion: The root of the issue is C++'s type system, fighting it is
clearly not simple.  Further, this is a known runtime problem that produces a
rather esoteric error.  What we're trying to protect against is less experienced
users. My proposal would be unless this quietly affects physics results, leave
it as is and make it very easy to find documentation on how to fix it in
multiple places FAQ, quickstart, usersguide, doxygen, etc.

Just my 2c,

    Dave

Chris Pollard <cpollard at cern.ch> writes:

> 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
>         
>
>
> _______________________________________________
> Rivet mailing list
> Rivet at projects.hepforge.org
> https://www.hepforge.org/lists/listinfo/rivet