Re: rhdb-admin

[Fernando Nasser <fnasser at redhat dot com>]

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