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

[Simon Glass]

Hi Albert,

On Fri, Oct 21, 2011 at 9:58 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Albert,
>
> On Fri, Oct 21, 2011 at 3:39 PM, Albert ARIBAUD
> <albert.u.boot at aribaud.net> wrote:
>> 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.
>
> I see few and small benefits. Of course if it is not there then people
> will not use it, so it is self-fulfilling.
>
>>
>>> 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.
>
> OK
>
>>
>>> 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.
>
> OK that's fine - I will redo the series without limits.h.

Done - sent as v5.

Regards,
Simon

>
> Regards,
> Simon
>
>>
>>> Regards,
>>> Simon
>>
>> Amicalement,
>> --
>> Albert.
>>
>