rhdb-admin

rhdb-admin

[Brett Schwarz]

No patches, but some recommendations:

1) Whenever you use 'expr', you should enclose the args in curly
brackets:

   expr 1 + 2 should be: expr {1 +2}

Same goes for 'if' statements, although I didn't explicitly see any...

This prevents double substitution (i.e. double processing). Not a big
thing, but if there are alot of them, it may add up in the performance.
This should work in most of the cases (there are some outliners).

2) To load Itcl, Itk, and Iwidgets, you only need to do package require
Iwidgets ... it loads Itk, and Itcl as well...

3) You also gain some by packing widgets together that have the same
pack options.

4) In some of the classes, the Itcl commands are not fully qualified,
and rhdb-admin doesn't even startup. I just added namespace import
::itcl::* to make it work, but maybe a better idea is to fully qualify
the Itcl commands, unless you know for sure there won't be any name
clashing. (I think the one I remember was configbody in
NewDisjointListBoxWidget class)

I haven't gone through thoroughly yet, but looks good...

HTH,

    --brett

p.s. more performance tips here: http://mini.net/tcl/348


-- 
Brett Schwarz
brett_schwarz AT yahoo.com

Re: rhdb-admin

[Fernando Nasser]

Brett Schwarz wrote:
> No patches, but some recommendations:
> 

Thanks for the hints and for taking the time to look into it.

> 1) Whenever you use 'expr', you should enclose the args in curly
> brackets:
> 
>    expr 1 + 2 should be: expr {1 +2}
> 
> Same goes for 'if' statements, although I didn't explicitly see any...
> 
> This prevents double substitution (i.e. double processing). Not a big
> thing, but if there are alot of them, it may add up in the performance.
> This should work in most of the cases (there are some outliners).
> 

All the if statements should have the conditions between braces as it is
the coding style used.

> 2) To load Itcl, Itk, and Iwidgets, you only need to do package require
> Iwidgets ... it loads Itk, and Itcl as well...
> 

Any harm in leaving them there?  The packages should all be loaded very
early in the execution anyway and having the explicit require better
documents things. Besides we may depend on a specific version of some
of these packages one day.


> 3) You also gain some by packing widgets together that have the same
> pack options.
> 

I do have the feeling that we have too many packages.  Maybe it is a
good idea to simplify this for next version.

> 4) In some of the classes, the Itcl commands are not fully qualified,
> and rhdb-admin doesn't even startup. I just added namespace import
> ::itcl::* to make it work, but maybe a better idea is to fully qualify
> the Itcl commands, unless you know for sure there won't be any name
> clashing. (I think the one I remember was configbody in
> NewDisjointListBoxWidget class)
> 

We fixed this already and will be checking this and a few other things in
cvs soon.  I run into this problem myself when I upgraded my version of
itcl/iwidgets.

> I haven't gone through thoroughly yet, but looks good...
> 

Thanks!

> HTH,
> 
>     --brett
> 
> p.s. more performance tips here: http://mini.net/tcl/348
> 
> 

Very interesting.  Thanks for the pointer.


Regards,
Fernando


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: rhdb-admin

[Brett Schwarz]

On Wed, 2002-10-23 at 12:47, Fernando Nasser wrote:
> Brett Schwarz wrote:

> 
> > 2) To load Itcl, Itk, and Iwidgets, you only need to do package require
> > Iwidgets ... it loads Itk, and Itcl as well...
> > 
> 
> Any harm in leaving them there?  The packages should all be loaded very
> early in the execution anyway and having the explicit require better
> documents things. Besides we may depend on a specific version of some
> of these packages one day.
> 

No, there is no harm. The package command won't load them again, if they
are already loaded, so there is no performance hit...

> 
> > 3) You also gain some by packing widgets together that have the same
> > pack options.
> > 
> 
> I do have the feeling that we have too many packages.  Maybe it is a
> good idea to simplify this for next version.

I was actually talking about the 'pack' geometry manager. For example,
if you have several widgets being packed:

    pack .w1
    pack .w2
    pack .w3
    pack .w4

This can be done as:
    pack .w1 .w2 .w3 .w4

If all of the pack options are the same (i.e. -side, -expand, etc). 

Even if one widget differs, it still might be advantageous to do:
    pack .w1
    pack .w2
    pack .w3
    pack .w4

    pack configure .w2 -fill both


As far as packages go, you might want to look into tclIndex instead of
using the package mechanism for internal "packages". The one advantage
is that it "loads on demand", so presumably, your startup time will be
less, and then you just take small incremental hits during interaction
with the user. However, it is a little more maintenance, since you have
to create the tclIndex file whenever you add/remove a proc/method/etc.
This is something you might want to investigate...might not make a
difference for this app...

> 
> > 4) In some of the classes, the Itcl commands are not fully qualified,
> > and rhdb-admin doesn't even startup. I just added namespace import
> > ::itcl::* to make it work, but maybe a better idea is to fully qualify
> > the Itcl commands, unless you know for sure there won't be any name
> > clashing. (I think the one I remember was configbody in
> > NewDisjointListBoxWidget class)
> > 
> 
> We fixed this already and will be checking this and a few other things in
> cvs soon.  I run into this problem myself when I upgraded my version of
> itcl/iwidgets.
> 

Cool...


Thanks,

    --brett

-- 
Brett Schwarz
brett_schwarz AT yahoo.com