On Fri, 2004-09-17 at 04:31 +0200, Christian Biere wrote:
> alex wrote:
> > Hmm, slight bug in src/ui/gtk/gtk2/bitzi.c, the table was missing some
> > comas :-(
>
> To prevent such bugs, it's best to use STATIC_ASSERT(). I guess the
> number of elements depends on some magic value?
Will do.
> > + "Unknown", /* UNKNOWN */
> > <SNIP>
> As these strings are user-visible they should should be marked translatable
> which is is simple as this: N_("Recommended"). The same applies to any
> other strings (except debug messages or warnings) presented to the user.
I figured they would have to be at some point, I'll redo this bit.
> <snip>
> If you don't need the table anywhere else, it should be put into the
> function. You should use a const qualifier for the table as long as you
> don't want to modify it at run-time. In the complete patch I've noticed that
> you use a similar table elsewhere. It's better to re-use this one here
> instead of using another one.
The table in core/bitzi.c is for enumerating the fixed XML tag into and
enum. If the the other table needs to a) provide translations, and b)
isn't in core I'm afraid some duplication will be required.
> > -#endif /* HAS_LIBXML2 */
>
> Hmm, if memory serves, it was decided that libxml2 is actually mandatory,
> so HAS_LIBXML2 is obsolete.
Erm, thats a - ;-)
> Looking at your complete patch, I've noticed the following:
>
> - There's a bug in bitzi_heartbeat().
There is? What exactly?
> - Please, stick to the style rules especially with respect to spaces
> before and after operators but also the position of curly braces. If
> you don't fix those, someone else has to this which is no fun.
I'll run it through indent before I submit the final patch.
> - IMHO, it's better to nest code as little as possible i.e., it's usually
> easier to read and you safe space by not using this
>
> void
> blah() {
> if (x) {
> if (y) {
> ...
> }
> }
> }
>
> but rather this:
>
> void
> blah() {
> if (!x) {
> ...
> return;
> }
> if (!y) {
> ...
> return;
> }
> ...
> }
>
> There are exceptions to every rule, of course.
Of course. I'm still from the school of multiple returns are bad
(although I break this from time to time to). I wasn't aware there was
anything to heavily nested (apart from xml parse code which could
probably be broken apart more).
> - Use G_FREE_NULL() instead of g_free() even if it might look ridiculous
> in straight-forward code. It's better to laugh now than crying later.
Why? g_free handles being passed NULL anyway.
> - You use guint32 for the filesize. This is in line with the rest of the
> code but it's bad, very bad. There are several options for gtk-gnutella:
Thats why I chose guint32, pick something for the rest of the core and
I'll change it. Its not used at the moment.
> <Snip>
> - Writing "No data yet" into the treestore might look nice from a user
> perspective but you should keep in mind that GTK+ will g_strdup() this
> string and will not use atoms. Thus you waste a couple of memory and
> what's worse performance for drawing the string (Pango is sloooow).
> As long as gtk-gnutella doesn't use a dedicated treemodel which uses
> atoms (which doesn't solve the Pango performance brake), you should
> rather pass a NULL pointer.
Ok, will do. Maybe it will be worth resurrecting some of the colour
change bits from the original patch?
> - You don't need g_strdup_printf() for "%s", "blah %s" or "blah". Just
> use g_strdup() which is faster and safer.
safer? Its only a g_strdup_printf for consistency with the other legs of
the code (IIRC it printed a little more details while I was testing).
> - In search_gui_metadata_update() you shouldn't copy the default string.
> As above, I would recommend using NULL instead. If you still want to
> use a default string, initialize "text" to NULL and pass a string
> literal to g_tree_store_set() like this:
> text ? text : "blah"
Ok
> - You use atom_.*_get() without using its return value. This might
> incidentially work in your case iff the passed pointer is already
> atomized but you should really use the return value. If you somehow
> rely that both pointers are identical you should use g_assert().
Fixed.
> - In bitzi_data_create() you allocate a new memory chunk and then
> initialize each single element. First, this looks like a good
> candidate for walloc(). Second, you should rather use
> "static const bitzi_data_default = { ... }" and make use of
> C's struct copy feature (or a plain memcpy - whatever floats your
> boat). This way, you can benefit from compiler diagnostics i.e.,
> "missing initializer" whenever you add more elements but might
> forget to update the constructor.
I'll clean that up.
> - For bitzi_cache_hashtable (could be a little shorter like
> bitzi_cache_ht), I think you should rely on the sha1 being an atom
> and simply pass NULL as hash and comparison function.
Uh, ok. Its the first time I used a glib hashtable so I probably looked
at some other code.
> - The bitzi URL shouldn't be inline in the code but rather at the
> top of some file as macro or static const string literal. Since
> the URL is rather short and has a fixed length, I would vote for using
> a fixed size buffer instead of g_strdup_printf().
I'll clean that up.
> - Not horribly important, but I mention it anyway: It looks like bitzi_rq
> should rather be a hashlist_t instead of a GSList as you make use of
> g_slist_append() which is O(n). This might be overkill and just caching
> the last element might be completely sufficient. gtk-gnutella needs
> more high-level lists anyway.
Its ordered and short (well as long as you don't select 1000 files at a
time, I may have to limit it) so I wasn't too worried.
> Nonetheless, the rest of your patch looks fine. I haven't tried it yet
> but I'm sure it'll be greatly appreciated by all users once integrated.
I'm way this weekend but I'll try and get most of the fixes in the next
patch submission sometime next week.
Thanks for the comments.
--
Alex, homepage: http://www.bennee.com/~alex/
A penny saved is a penny to squander. -- Ambrose Bierce
-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php
_______________________________________________
Gtk-gnutella-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/gtk-gnutella-devel
