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.b...@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.b...@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.b...@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<s...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
