Hi Albert,
On Fri, Oct 21, 2011 at 3:39 PM, Albert ARIBAUD albert.u.boot@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@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@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@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 Glasssjg@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.