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

[Simon Glass]

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.

Regards,
Simon

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