Hi Gerlando, On Mon, Dec 12, 2011 at 1:32 AM, Gerlando Falauto <gerlando.fala...@keymile.com> wrote: > On 12/07/2011 11:02 PM, Simon Glass wrote: > > [...] >>> >>> /* >>> - * Set a new environment variable, >>> - * or replace or delete an existing one. >>> + * Performs consistency checking before setting, replacing, >>> + * or deleting an environment variable, then (if successful) >>> + * apply the changes to internals so to make them effective. >>> + * Code for this function was taken out of _do_env_set(), >>> + * which now calls it. >>> + * Also called as a callback function by himport_r(). >>> + * Returns 0 in case of success, 1 in case of failure. >>> + * When (flag& H_FORCE) is set, force overwriting of >>> + * write-once variables. >> >> >> can you word-wrap that to 72 columns perhaps? > > > Sorry, I am little confused now. What is the maximum line length?
What I meant was that it seems like it you could put more characters on each line, and then would take up less vertical space. You could also use the doxygen @param formal to make it quicker for people to read. > > [...] > > >>> +/* Check whether variable name is amongst vars[] */ >>> +static int process_var(const char *name, int nvars, char * const vars[]) >>> +{ >>> + int i = 0; >> >> >> blank line here. > > > Thanks, I didn't know about this. > > >> Can part of this function be #ifdefed to reduce code >> size when the feature is not required? > > > Uhm, I don't think so. This would be a common feature for selectively > importing & setting back to default. Unless we want to #ifdef both of them. > Personally, I would #ifdef neither. OK, just a thought, it is up to you. > > [...] > >> >> + if (!process_var(name, nvars, vars)) >> + continue; >> >> if (hdelete_r(name, htab) == 0) >> debug("DELETE ERROR >> ##############################\n"); >> >> perhaps: >> >> if (process_var(name, nvars, vars)&& >> hdelete_r(name, htab) == 0) >> debug("DELETE ERROR ##############################\n"); > > > > I think it's easier to read it the original way, and it should not make any > difference as far as code size is concerned. OK > > >> himport_r() is getting a bit overloaded, > > > Actually, I believe it makes no longer sense to have it called "_r", as it > was the original reference to the function being recursively calleable (i.e. > reentrant) as opposed to other versions which were not. OK I'm not sure about that. > > >> and it's a shame but I can't >> think of an easy way to refactor it to reduce the number of args. In a >> way you are adding a processing option and so you could put the three >> extra args in a structure and pass a pointer to it (or NULL to skip >> this feature). Not sure though... > > > Can't think of any other way either, except maybe renaming it and > re-implementing the shorter version as a wrapper around the newly named > function. I had already done that, but there would be very few places where > the old syntax would be kept, so it just didn't make much sense. Packaging up a lot of zero arguments to a function does chew up code space. > > >> Also, for me this patch adds 500 bytes. I wonder if more of the code >> could made optional? > > > Frankly, I'm surprised to hear this adds that much overhead. > Actually, I can't see this increase in code size as you mention. > What architecture are you referring to? ARMv7. Ideally if your feature is optional it shouldn't add much size when it is turned off. > In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are > surprisingly unchanged in size, even with debug #defined! > Only place I can experience such growth is within unstripped u-boot ELF, > which I believe shouldn't really matter... or should it? As Wolfgang says, use your tool chain's size tool for this. Regards, Simon > > Best, > Gerlando _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot