[U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes

[Simon Glass]

Hi Gerlando,

On Mon, Dec 12, 2011 at 1:32 AM, Gerlando Falauto
<gerlando.falauto at 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