- Dillo-dev - lists.mailman3.com

Style question: cleaning up NULL

by onepoint＠starurchin.org

On Sun, May 03, 2009 at 09:35:34AM -0400, Jorge Arellano Cid wrote: > On Sun, May 03, 2009 at 11:27:14AM +0100, Jeremy Henty wrote: > > > > ... a_Url_free() and a_Image_unref() both guard against being > > passed a NULL argument, yet in several cases the code that calls > > these functions also checks for a NULL, which is redundant. > > In general the API accepts NULL (e.g. dlib), so please remove the > redundant checks outside it. OK, patches attached. Regards, Jeremy Henty

May 3, 2009

[Patch 3/4] Key bindings

by jcid＠dillo.org

Hi, A new, rewritten version of the patch was committed. It is O(log n) instead of O(n^3), supports named keys, and multiple keybindings. It supports the traditional key bindings by default, and they can be configured in ~/.dillo/keysrc. e.g. <Ctrl>a = bookmarks F6 = back . = forward <Ctrl>b = back The patch is not 100% complete. For instance there's a need to provide a default keysrc file in a similar way that cookiesrc or dillorc (with useful comments). Please test it and polish where needed. It has had very little testing. Note: O(log n) may be an overkill, but once it was O(n) it was easy to use dlib's sorted list instead of plain list. It can also be used as a template for other sources. -- Cheers Jorge.-

May 3, 2009

Style question: cleaning up NULL

by jcid＠dillo.org

On Sun, May 03, 2009 at 11:27:14AM +0100, Jeremy Henty wrote: > > I've noticed that a_Url_free() and a_Image_unref() both guard against > being passed a NULL argument, yet in several cases the code that calls > these functions also checks for a NULL, which is redundant. I think > that if the API officially accepts NULL then code that calls the API > shouldn't be checking for it. Conversely, if the API does not accept > NULL then its implementation shouldn't be checking for it. So I would > suggest we remove the redundancy by eliminating the NULL check from > either the callers or the API implementation. Thoughts? I'll provide > patches if people agree and decide whether or not these APIs should > support NULL. In general the API accepts NULL (e.g. dlib), so please remove the redundant checks outside it. -- Cheers Jorge.-

May 3, 2009

Style question: cleaning up NULL

by onepoint＠starurchin.org

I've noticed that a_Url_free() and a_Image_unref() both guard against being passed a NULL argument, yet in several cases the code that calls these functions also checks for a NULL, which is redundant. I think that if the API officially accepts NULL then code that calls the API shouldn't be checking for it. Conversely, if the API does not accept NULL then its implementation shouldn't be checking for it. So I would suggest we remove the redundancy by eliminating the NULL check from either the callers or the API implementation. Thoughts? I'll provide patches if people agree and decide whether or not these APIs should support NULL. Regards, Jeremy Henty

May 3, 2009

[patch]: fix comment for a_Cache_open_url()

by onepoint＠starurchin.org

a_Cache_open_url() never returns 0, so its comment needs fixing. (Bonus typo fix included too.) Regards, Jeremy Henty

May 3, 2009

[patch]: ensure Cache_client_make_key() returns > 0

by onepoint＠starurchin.org

Cache_client_make_key() carefully prevents integer overflow from making it return a negative value, but it might return 0. This is a problem because a_Capi_open_url() either calls a_Cache_open_url() and returns the new client key or else returns 0 to signal that it did not call a_Cache_open_url(). If Cache_client_make_key() returns 0 then a_Capi_open_url() will return 0 too, and so its caller will think that a_Cache_open_url() was not called even though it was. This will stop the new cache client from being added to the browser window that is responsible for creating it, which is wrong. Regards, Jeremy Henty

May 3, 2009

[patch]: fix comment to a_Html_image_new()

by onepoint＠starurchin.org

On Fri, May 01, 2009 at 01:52:31PM +0000, corvid wrote: > (I thought the line was "Does what it says on the tin.") Yes it is, but I avoid using such phrases in case it causes confusion. But if you'd like me to be more colloquial, I will! :-) Regards, Jeremy Henty

May 2, 2009

[patch]: fix comment to a_Html_image_new()

by corvid＠lavabit.com

Jeremy wrote: > Does what the subject says. (I think this is my fault: I proposed > taking out the "add" argument and renaming the function but forgot the > revise the comment.) Committed. (I thought the line was "Does what it says on the tin.")

May 1, 2009

[patch]: fix comment to a_Html_image_new()

by onepoint＠starurchin.org

Does what the subject says. (I think this is my fault: I proposed taking out the "add" argument and renaming the function but forgot the revise the comment.) Regards, Jeremy Henty

May 1, 2009

[Patch 2/4] Refactoring src/prefs.c and src/dir.c

by jcid＠dillo.org

Hi Tim, On Tue, Apr 21, 2009 at 03:48:49PM +0000, Tim Nieradzik wrote: > > The patch has a strange comment WRT equal signs: > > > > + // parse the value > > + i = 0; > > + while (*p) { > > + // skip all spaces and equal signs > > + if (!isspace(*p)) { > > + value[i++] = *p; > > + } > > + > > + p++; > > + } > > + > > + value[i] = '\0'; > > > > So either the implementation or the comment is wrong... > > The comment is wrong, I am sorry. If you look a few lines above, you > will see that we have already checked whether the current character > is an equal sign. > > In a previous implementation I used to have an additional check > there but removed it later as we do not want to skip any equal signs > within values. Equal signs are very common in search URLs so > skipping them could have led to many problems. > > I guess I just forgot to adjust the comment accordingly after having > changed this part. > > > If the required job is basically the same, I can keep the current > > function (maybe returning the new termination codes) and continue > > integrating the patchset. > > Yes, that is probably best. OK, a new patch for this was just committed. Several changes/fixes were made. Of the top of my head: - Remove multiple return points inside functions - Comments accuracy - Keep source code file margin < 80 cols. - Identation style in dillo uses 3 spaces. (and there was wrong indentation too). - FIxed a memory leak. Tim: please make a diff between a tree with the original patch and what was committed to get a detailed view of the changes. I'll continue integrating from here. -- Cheers Jorge.-

April 28, 2009