[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Albert ARIBAUD]

Le 22/10/2011 00:02, Simon Glass a écrit :
> Hi Albert,
>
> On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD
> <albert.u.boot at aribaud.net>  wrote:
>> Le 21/10/2011 23:12, Simon Glass a écrit :
>>>
>>> Hi Albert,
>>>
>>> On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD
>>> <albert.u.boot at aribaud.net>    wrote:
>>>>
>>>> Le 21/10/2011 22:19, Simon Glass a écrit :
>>>>>
>>>>> Hi Albert,
>>>>>
>>>>> On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD
>>>>> <albert.u.boot at aribaud.net>      wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> Le 10/10/2011 21:22, Simon Glass a écrit :
>>>>>>>
>>>>>>> This brings a basic limits.h implementation into U-Boot.
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass<sjg at chromium.org>
>>>>>>> ---
>>>>>>>   fs/ubifs/ubifs.h |    4 +---
>>>>>>>   include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
>>>>>>>   2 files changed, 41 insertions(+), 3 deletions(-)
>>>>>>>   create mode 100644 include/limits.h
>>>>>>
>>>>>> Apparently, in all the U-Boot codebase, only one file required standard
>>>>>> limit names, and gets them with three lines of code. Is it worth adding
>>>>>> 40
>>>>>> lines of code for this?
>>>>>
>>>>> Well 2/3 is the license header - and I thought it best to add all the
>>>>> limits in one go. I can add just those few if you like.
>>>>>
>>>>> This file is used later in the patch series.
>>>>
>>>> I don't see much use of these in the subsequent patches either -- and
>>>> those
>>>> few uses could be discussed, such as for instance the use of INT_MAX as
>>>> the
>>>> maximum buffer size for some *printf() functions -- actually, anything
>>>> very
>>>> big would fit just as well, would it not?
>>>
>>> Yes it would, it's doesn't really need to be INT_MAX. Then again,
>>> limits.h is a fairly standard file to have around, and INT_MAX is an
>>> efficient 'large' value to load on many architectures.
>>>
>>> In any case it seems wrong to me that ubifs is defining its own
>>> INT_MAX and U-Boot doesn't have one.
>>
>> My point is precisely that as standard as limits.h is, U-Boot has managed to
>> survive not having it around so far, which kind of shows limits.h is not
>> needed.
>
> By that logic one would never do any code clean ups. Do I understand
> you correctly?

You're extending my logic here: not all cleanups are done by adding a 
header file and replacing some lines by an include and some other lines. :)

Actually, I don't think introducing limits.h is any cleanup; it is an 
attempt at using standards whenever possible, which may be fine with 
some standards: I'd be happy of U-Boot used uint{8,16,32}_t instead of 
u{8,16,32}, for instance, because it uses that a lot. With limits.h, my 
gripe with it here is that, while possible, I don't see it bringing 
benefits here as 1) the ubi code already defines what it needs, 2) other 
uses in the patchset do not actually require /limits/, and 3) there are 
not many places in the whole U-Boot code that do.

If you can prove me wrong, i.e. if you can show that limits.h would add 
a lot of benefits to more than the other files in its own patchset, then 
I'll happily reconsider.

> But this is the least of my concerns :-) If you don't want it, don't
> take it. Shall I modify the series to define its own INT_MAX, or just
> chose a large number?

Well I don't think the limits.h introduction is useful here, and not 
introducing it will barely cost a source code line. As for the number to 
use in *printf(), either way is fine with me, as this number is 
arbitrary anyway.

> BTW I think you are looking at the old version of that patch series -
> we are now on v4. The limits.h patch is the same though. Later on in
> the series I add comments to vsprintf() functions and move them into
> their own header. If you apply the same logic there then that tidy is
> not needed also. Please let me know.

Thanks for reminding me. I did see the V4 series and it is the one I 
actually commented on in my previous mail. Apologies for not having made 
that explicit.

> Regards,
> Simon

Amicalement,
-- 
Albert.