[prev in list] [next in list] [prev in thread] [next in thread] 

List:       elinks-dev
Subject:    [elinks-dev] Re: [patch] Store builtin options in tables
From:       Petr Baudis <pasky () ucw ! cz>
Date:       2003-10-23 21:10:51
Message-ID: 20031023211051.GY1075 () pasky ! ji ! cz
[Download RAW message or body]

Dear diary, on Thu, Oct 23, 2003 at 07:53:48PM CEST, I got a letter,
where Jonas Fonseca <fonseca@diku.dk> told me, that...
> This patch gives us another decrease in the option systems memory usage:
> 
> 23921 jonas      9   0  3544 3540 1700 S  0.0  1.4   0:00.18 elinks
> 23924 jonas      9   0  3628 3624 1732 S  0.0  1.4   0:00.18 elinks-old
> 
> and start up time but also a minor decrease in the size of the binary:
> 
> -rwxr-xr-x    1 jonas    jonas     3314189 Oct 23 19:31 elinks
> -rwxr-xr-x    1 jonas    jonas     3323943 Oct 23 17:36 elinks-old
> 
> The idea is to make (the newly added) config/options.inc a ``pure data''
> file (like all the other *.inc files). This means that we no longer need
> to allocate builtin options (besides _template_ ones) at startup but rather
> just iterate two tables (one for config options and one for command
> line options) and build the option tree.
> 
> The patch will need a lot of cleanup patches to follow it up and make
> the option system look nice again.
> 
> I have snipped a lot of trivial lines which basicly do :s/;$/,/ in
> config/options.inc .. also there should be a few lines that can be
> factored out (list_box initialization) and shared with the current
> option adding code.

It looks good, almost approved.

> Index: config/options.h
> ===================================================================
> RCS file: /home/cvs/elinks/elinks/src/config/options.h,v
> retrieving revision 1.77
> diff -u -d -r1.77 options.h
> --- config/options.h	23 Oct 2003 15:03:30 -0000	1.77
> +++ config/options.h	23 Oct 2003 17:35:21 -0000
> @@ -44,6 +44,9 @@
>  	 * on the tree argument to add_opt, it will create the listbox (options
>  	 * manager) item for the option. */
>  	OPT_LISTBOX = 16,
> +	/* This is used to mark that the option is builtin and should therefore
> +	 * not be free()d. */
> +	OPT_BUILTIN = 32,
>  };
>  
>  enum option_type {

This is my main problem, OPT_BUILTIN is too general and confusing.
Instead, please use OPT_ALLOC or so. Always name the flags as specific
as possible, tying only one possible effective meaning to them.

-- 
 
				Petr "Pasky" Baudis
.
To get something done, a committee should consist of no more than three
persons, two of them absent.
.
Stuff: http://pasky.ji.cz/

[prev in list] [next in list] [prev in thread] [next in thread]