[Dillo-dev]Patch review: Frank de Lange patch (20031124)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hello, I am dissecting and reviewing Frank de Lange's 14,000 line patch that was rejected by the core dillo developers due to its size. I will email the list from time to time to gather input as to whether a particular portion of the patch is useful or not, beginning with the patch a little further down this message. The following patch does does two things in selection.c: - - make a_Selection_set_dclick_callback replace the callback instead of returning when the callback was already set. * This function is called only once in the code (in interface.c); the callback pointer always points to the same function. So I don't see a point in merging this first hunk, although it won't do any harm. Thoughts? - - Replace three comparison operators (==) with bitwise AND.. * This only works as expected when either lvalue and rvalue have exactly one or zero bits set to 1. Although this currently is true, this may change in the future; thus, I don't think these three hunks should be merged. Regards, Bastiaan Jacques. diff -Nur --exclude-from /home/andi/freeciv/diff_ignore dillo-bak/src/selection.c dillo/src/selection.c - --- dillo-bak/src/selection.c 2004-02-20 14:28:59.000000000 +0100 +++ dillo/src/selection.c 2004-02-20 14:30:17.000000000 +0100 @@ -76,7 +76,10 @@ ~ void (*fn) (gpointer data), ~ gpointer data) ~ { - - g_return_if_fail (selection->dclick_callback == NULL); + /* this used to return when the callback was already set, + * changed function to replace callback instead. + * g_return_if_fail (selection->dclick_callback == NULL); + */ ~ selection->dclick_callback = fn; ~ selection->callback_data = data; ~ } @@ -426,7 +429,7 @@ ~ DwIterator *top = it->stack[it->stack_top]; ~ gint len; - - if (top->content.type == DW_CONTENT_TEXT) + if (top->content.type & DW_CONTENT_TEXT) ~ len = strlen(top->content.data.text); ~ else ~ len = 1; @@ -486,7 +489,7 @@ ~ for (i = a_Dw_ext_iterator_clone (a), start = TRUE; ~ (cmp = a_Dw_ext_iterator_compare (i, b)) <= 0; ~ a_Dw_ext_iterator_next (i), start = FALSE) { - - if (i->content.type == DW_CONTENT_TEXT) { + if (i->content.type & DW_CONTENT_TEXT) { ~ if (fl) { ~ if (start) { ~ DEBUG_MSG (2, " highlighting %s from %d to %d\n", @@ -537,7 +540,7 @@ ~ cmp = a_Dw_ext_iterator_compare (selection->from, selection->to); ~ if (cmp == 0) { - - if (selection->from->content.type == DW_CONTENT_TEXT) { + if (selection->from->content.type & DW_CONTENT_TEXT) { ~ si = selection->from->stack[selection->from->stack_top]; ~ if (selection->from_char < selection->to_char) ~ tmp = g_strndup(si->content.data.text + selection->from_char, -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.0 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFB77I5wDeUwhiOHwURAkYeAKCzu03fggXw2DvdEiBWAP4GicLw5wCggUid Kg67vhiAUHc2dK+jGGVtE9o= =AGq9 -----END PGP SIGNATURE-----
Bastiaan Jacques wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hello,
I am dissecting and reviewing Frank de Lange's 14,000 line patch that was rejected by the core dillo developers due to its size. I will email the list from time to time to gather input as to whether a particular portion of the patch is useful or not, beginning with the patch a little further down this message.
Is this patch 0.8.4 ready? Is this patch available? May try it and check the code it modified? Cheers, -- Roberto A. Foglietta Analista Programmatore GNU/Linux SAD Trasporto Locale S.p.a. Corso Italia 13/N 39100 BOLZANO (I) Tel. +39/0471-450.261 Fax +39/0471-450.253
Hi Bastiaan, On Thu, Jan 20, Bastiaan Jacques wrote:
The following patch does does two things in selection.c: - - make a_Selection_set_dclick_callback replace the callback instead of returning when the callback was already set. * This function is called only once in the code (in interface.c); the callback pointer always points to the same function. So I don't see a point in merging this first hunk, although it won't do any harm. Thoughts?
I'd rather leave it the current way, since changing the callback as in your patch would mean, that the old callback would be silently deactivated. To extend the current functionality, a remove function may be useful (to make deactivation explicit), or even a list of callbacks, so that the functions do not have to be removed (unless they should so). Anyway, there is currently no demand for an extended functionality, so I'd leave it as simple as it is now.
- - Replace three comparison operators (==) with bitwise AND.. * This only works as expected when either lvalue and rvalue have exactly one or zero bits set to 1. Although this currently is true, this may change in the future; thus, I don't think these three hunks should be merged.
This (that only one bit set) will also be true in the future. If contents will be extended to contain multiple types (which is not planned at all), the whole design must be changed anyway (at least, the unions will have to replaced by structs), so grepping over the code is anyway necessary. Until then, your suggestion would only make the code more confusing. Sebastian
participants (3)
-
Bastiaan Jacques -
Roberto A. Foglietta -
Sebastian Geerken