[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-multimedia
Subject:    Re: Debugging smartwrappers
From:       Nicolas Brodu <nicolas.brodu () free ! fr>
Date:       2000-03-19 22:13:15
[Download RAW message or body]

Stefan Westerfeld wrote:
> 
>    Hi!

Hi!

[snip]
> 
> The smartwrappers now also no longer use _var's internally, but _base classes
> and do reference counting themselves. This is better because in the upper
> case, having _vars would result in referencing the object twice and
> dereferencing it twice (as you have one _var per smartwrapper).

Nifty thing! I thought about having a _var only in the topmost parent, but then
there was a problem for multiple inheritance. I point this now, because
depending on your implementation we could have the same problem here.
At first sight, it seems correct though.

> 
> I am still a bit suspicious of the next change I made: on demand creation.
> This allows you to do the following
> 
> StereoEffect effect = /* something that returns an effect here */;
> 
> without having the effect created and shortly after that destroyed. However
> 
> Synth_PLAY p;
> p.start();
> 
> will make the on demand creation kick in in the second line, so that no
> difference to the former behaviour is visible. What do you think?

Well, the first line uses a constructor, the second an operator.
In both case I think this new implementation has a problem:

inline StereoEffect(StereoEffect& target) : _StereoEffect_redirect(0) {
   _assign_StereoEffect_base(target._StereoEffect_base()->_copy());
}

The call to target._StereoEffect_base() forces the lazy creation.

If you do:
  StereoEffect effect;
  StereoEffect effect2 = effect;
Then effect and effect2 will be both created, which violates the lazy concept.

I think the solution is:

inline StereoEffect(StereoEffect& target) : _StereoEffect_redirect(0) {
  if (target._StereoEffect_redirect) {
     _assign_StereoEffect_base(target._StereoEffect_base()->_copy());
  }
}

So that you don't modify _StereoEffect_redirect and _autoCreate (you do nothing,
like in the () constructor).

Same thing for the operator.

> 
> However, all that reference counting results in having many operations non-
> const, although they ought to be const (such as assignment operators and such).
> If anybody knows a brilliant solution, let me know.

I don't know now, just looking in a few minutes. We'll see when I go back
home...

> With these changes, smartwrappers have a chance again to become universal
> usable, that means, they maybe could replace all CORBA _var, _copy(),
> _release(), _create(), _base* etc. stuff in the code.

That would be really great. If we could solve the problems presented in your
last mails, it's fine!

> I am sorry if my position towards what shall become the MCOP api in the
> future currently looks a bit like one day like this, the other day like that.

AAArghl. I've spent a few hours today reverting kdelibs to the old syntax...
Well, it's for the best!

> However, how this is done best is a non-trivial thing to decide.
> 
> The arguments currently look like:
> 
>   pro "only smartwrappers"
> 
> - smartwrappers are easier to use

Sure.

> - they are - now - probably usable everywhere where CORBA syntax is usable

That was the big no-no. I'd also like to add that SmartWrappers allow
CORBA-unaware developpers to work directly, without first learning how things
work (why a _var, _skel...).

> - they might avoid stupid mistakes you otherwise run into

Important point.

>   contra "only smartwrappers"
> 
> - smartwrappers increase code size & compile time

They increase compile time and the size of the source code. The generated code
is as fast as before, but introduce an extra virtual table for the connect.
Well, with your new great code, we have the virtual destructor also. Still, this
doesn't affect at all the communication speed (streaming).

> - their on-demand-creation semantics is a bit strange

What do you mean?

I'll add another bad point: error handling. If you do 'Foo foo;' and an
implementation doesn't exist you have a null object. Usually, you assume foo is
well defined after such a line...

Perhaps we could put all the error conditions (isNull(), _error(), ...) in a
single error() function returning 0 if OK or an error code otherwise. I don't
think exceptions would be better. You could then do:
  Foo foo;
  if (!foo.error()) {/*blah*/}

Better than nothing.

> 
> I would be glad if anybody could proof-read my smart wrapper changes.

I'll do so. I've just downloaded the sources. Hot comments:
- I'm still not sure about this inline virtual business. I'd feel safer with
non-inline virtuals, coded in a separate .cc file.
- The lazy creation is great.

Some comments of my own coding today:

- Found a solution for the parameter and return values using SmartWrappers.

Following a previous mail:
  /* idl code */
  Foo foo(Foo foo);

maps to:
  Foo_base *foo(Foo_base *foo);

Using SmartWrappers, it seems logical/clearer that it maps instead to:
  Foo foo(Foo& foo);

I also overloaded with:
  Foo foo(Foo_var foo);

To be able to pass Foo_base* and Foo_var if necessary (ex, in artscat).

Now, the good news is that with your lazy creation the returned Foo is quite
small and does nothing so long as it's not used, so there is even less reason
for returning a Foo*. The overloading isn't necessary anymore as well since you
have a _base* cast operator (wasn't possible before because of the
double-wrapping effect).

- 'component' keyword: Useless, really.

- namespace: In the reversed version I have, there is no _base. Instead the
smartwrapper is placed in a namespace.
Problem, code like this doesn't work:
  using namespace MCOPComponent;
  Foo foo;
because there is still confusion between ::Foo and ::MCOPComponent::Foo for the
compiler. You have to use {} for the namespace each time, which isn't that
great.
Now, if we keep the _base, the namespace isn't needed.

- remote connection: What about adding a class GlobalSoundServer that would
always be a reference to "global:Arts_SimpleSoundServer" ?

That way:
  SimpleSoundServer server(Reference("global:Arts_SimpleSoundServer"));

Could be written:
  GlobalSoundServer server;

If later on you change the reference string (for instance,
"global:Arts_SoundServer") then it's binary compatible in the client apps.

Moreover, this line is necessary, so it would avoid code duplication.

OK, so now, I'll look at your code and merge my changes.

Cheers,
Nicolas
-- 
A shortcut is the longest distance between two points. (unknown author)

[prev in list] [next in list] [prev in thread] [next in thread]