[Clam-devel] TypedControls Patch

[Pau Arumí]

On dv, 2008-05-09 at 16:36 -0300, Francisco Tufró wrote:
> Passing a class by copy may bring performance issues on realtime i
> think, that's why i did it by reference.
> If you want it to be by copy, i have no problem to do it by copy.

Yes because a reason, let me explain:
As you say, copies can be expensive. It depends on the object size. And
things could be MUCH worst if a user makes a TypedControl class of
hierarchic objects with deep-copy semantics requiring memory allocation.
But using such (deep or big) objects as controls is a bad use of
controls, and this should be documented (at least at the doxygen class
doc)

But the alternative of keeping a pointer to the original object is much
worst. Picture this we have a processing A sending controls to B.
A::Do((
  MyType value = someVal();
  outcontrol.SendControl(value);

This would cause a segfault when B reads the received control, because
it points to non-existing var (it is out of scope).

Restricting to send only attributes would solve this, but can not be
enforced. And it would also be a bad solution, it totally breaks
encapsulation (A and B shares the same variable).

Some alternatives i'm thinking on now:
1. Passing a copy-on-write smart-pointer. I think would be over-design
at this stage. Maybe later.
2. Do not save the received value by default. Just provide a callback
method and the class decides if the value needs to be copied, to be
read, or to be sent out to another linked control (cascading events)

Maybe it would be worth to do 2. But if in most cases the receiver just
needs to keep a copy (and wait for its Do) then it's over-design. I
would first implement the "copy" version.

I'm interested to hear more opinions on 2, (David, specially)

Pau 
 

  



> For the next patch i'll change it and add the IsConnected test and
> pass it.
> that's all for now
> 
>  
> On 5/9/08, Pau Arumí <parumi at iua.upf.edu> wrote: 
>         On dj, 2008-05-08 at 14:43 -0300, Francisco Tufró wrote:
>         > After some hours of work i have done 2 tests and passed
>         them.
>         > Just check them and tell me if they're ok (and if the patch
>         is well
>         > done ^^)
>         > :)
>         
>         Good start! I see the code checked in by Hernan.
>         It's is good to go in small steps. Even this simple test
>         brings out some
>         issues:
>         
>         
>         +               {
>         +                       CLAM::TypedInControl<int>
>         in("IntInControl");
>         +                       CLAM::TypedOutControl<int>
>         out("IntOutControl");
>         +                       out.AddLink(in);
>         +                       int number=1;
>         +                       out.SendControl(number);
>         +                       CPPUNIT_ASSERT_EQUAL( 1 ,
>         in.GetLastValue() );
>         +               }
>         +
>         
>         out.SendControl(1); won't work because the signature wants a
>         reference
>         to a non-constant var! And we need it to work, for backwards
>         compatibility among other reasons.
>         
>         So change the test passing a constant, and pass the test by
>         changing
>         SendControl and DoControl arguments to be const references.
>         
>         We want controls to be passed by copy (or maybe in the future:
>         copy-on-write smart pointers), but keeping a reference to the
>         original
>         object is bad.
>         
>         I was considering a new "sent by copy" test (e.g. send a var,
>         change the
>         var, check that the received value has not changed) but i
>         realize it is
>         not necessary, once we have changed the previous test: if you
>         save a a
>         const argument it necessarily implies doing copy.
>         
>         > When you give me the ok, i'll continue with the
>         > "TypedInControl::IsConnected(out)" test.
>         > Bye!
>         
>         Yes
>         
>         Pau
>         
>         
>         _______________________________________________
>         Clam-devel mailing list
>         Clam-devel at llistes.projectes.lafarga.org
>         https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel
> 
> _______________________________________________
> Clam-devel mailing list
> Clam-devel at llistes.projectes.lafarga.org
> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel