small patch for prototypes and unused variables

[Maciej Pijanka]

Hello

see patch in attachment,

best regards
Maciej

[Maciej Pijanka]

Hello

as Uwe sugested, i checked abuild -a, and found one romcc prototype
problem, fixed patch in attachment.

best regards
Maciej

[Myles Watson]

Maciej,

Thanks for the patch.  I think most of it is ready to be committed.

Index: src/lib/clog2.c
===================================================================
--- src/lib/clog2.c	(revision 4869)
+++ src/lib/clog2.c	(working copy)
@@ -7,6 +7,8 @@
 /* Assume 8 bits per byte */
 #define CHAR_BIT 8

+unsigned long log2(unsigned long x);
+
 unsigned long log2(unsigned long x)
 {
         // assume 8 bits per byte.

Things like this make me wonder if we should just turn off the warning.  Is
there a header file where we can put some of these prototypes?

Thanks,
Myles

[Maciej Pijanka]

On 27/10/2009, Myles Watson  wrote:
> Maciej,
>
> Thanks for the patch.  I think most of it is ready to be committed.
> Index: src/lib/clog2.c
>
> --- src/lib/clog2.c	(revision 4869)
> +++ src/lib/clog2.c	(working copy)
> @@ -7,6 +7,8 @@
>  /* Assume 8 bits per byte */
>  #define CHAR_BIT 8
>
> +unsigned long log2(unsigned long x);
>
> unsigned long log2(unsigned long x)
>  {
>          // assume 8 bits per byte.
>
> Things like this make me wonder if we should just turn off the warning.  Is
> there a header file where we can put some of these prototypes?

On begining of previous patch version, was an comment about such
things, i didn't found
suitable header where such utility function prototypes might be added,
if its desired to make
include/utils.h or something similar with better name i will move
prototypes there.

But please note in few places prototype is just before function
because it is used only locally (for my knowledge) if not then it
should be moved to header too.

So if you have idea how this issue should be fixed let me know.

Maciej

[Myles Watson]

On Tue, Oct 27, 2009 at 7:32 AM, Maciej Pijanka wrote:
> On 27/10/2009, Myles Watson  wrote:
> > Maciej,
> >
> > Thanks for the patch.  I think most of it is ready to be committed.
> > Index: src/lib/clog2.c
> >
> > --- src/lib/clog2.c   (revision 4869)
> > +++ src/lib/clog2.c   (working copy)
> > @@ -7,6 +7,8 @@
> >  /* Assume 8 bits per byte */
> >  #define CHAR_BIT 8
> >
> > +unsigned long log2(unsigned long x);
> >
> > unsigned long log2(unsigned long x)
> >  {
> >          // assume 8 bits per byte.
> >
> > Things like this make me wonder if we should just turn off the warning.
>
> Is
> > there a header file where we can put some of these prototypes?
>
> On begining of previous patch version, was an comment about such
> things, i didn't found
> suitable header where such utility function prototypes might be added,
> if its desired to make
> include/utils.h or something similar with better name i will move
> prototypes there.

Sorry, I didn't look at the first version of the patch.  I think that the
cleanest thing would be to have files in include that mirror the .c files.
If we have to add too many new .h files, maybe we should combine some .c
files.

But please note in few places prototype is just before function
> because it is used only locally (for my knowledge) if not then it
> should be moved to header too.

If you declare it as static that will silence the warning as well.  Of
course this only works when it is just used locally.

Thanks,
Myles

[Maciej Pijanka]

On 27/10/2009, Myles Watson  wrote:
> On Tue, Oct 27, 2009 at 7:32 AM, Maciej Pijanka
> wrote:
> > On 27/10/2009, Myles Watson  wrote:
> > > Maciej,
> > >
> > > Thanks for the patch.  I think most of it is ready to be committed.
> > > Index: src/lib/clog2.c
> > >
> > > --- src/lib/clog2.c   (revision 4869)
> > > +++ src/lib/clog2.c   (working copy)
> > > @@ -7,6 +7,8 @@
> > >  /* Assume 8 bits per byte */
> > >  #define CHAR_BIT 8
> > >
> > > +unsigned long log2(unsigned long x);
> > >
> > > unsigned long log2(unsigned long x)
> > >  {
> > >          // assume 8 bits per byte.
> > >
> > > Things like this make me wonder if we should just turn off the warning.
> >
> > Is
> > > there a header file where we can put some of these prototypes?
> >
> > On begining of previous patch version, was an comment about such
> > things, i didn't found
> > suitable header where such utility function prototypes might be added,
> > if its desired to make
> > include/utils.h or something similar with better name i will move
> > prototypes there.
>
> Sorry, I didn't look at the first version of the patch.  I think that the
> cleanest thing would be to have files in include that mirror the .c files.
> If we have to add too many new .h files, maybe we should combine some .c
> files.

I split patch, leaving part that don't require adding new headers as
one, while rest is in another.
> But please note in few places prototype is just before function
> > because it is used only locally (for my knowledge) if not then it
> > should be moved to header too.
>
> If you declare it as static that will silence the warning as well.  Of
> course this only works when it is just used locally.

I checked using grep, and it seems that resource_tree is called just
from device_util.c where its declared, so might be static and moved to
not-requiring new headers part of patch.
> Thanks,
> Myles

[Myles Watson]

Acked-by: Myles Watson

Thanks,
Myles

[Myles Watson]

On Tue, Oct 27, 2009 at 8:20 AM, Myles Watson  wrote:
> Acked-by: Myles Watson

Rev 4871.

Thanks,
Myles

[ron minnich]

In v3 IIRC we decided on an include file, lib.h, which had these sorts
of "nuisance" prototypes.

ron

[Myles Watson]

On Tue, Oct 27, 2009 at 8:58 AM, ron minnich  wrote:
> In v3 IIRC we decided on an include file, lib.h, which had these sorts
> of "nuisance" prototypes.

Something like that would be fine with me, but since we'd do it just to do
it, not to make the code cleaner, maybe it's just as good to put the
prototypes by the functions.

Thanks,
Myles

[ron minnich]

On Tue, Oct 27, 2009 at 8:17 AM, Myles Watson  wrote:
> Something like that would be fine with me, but since we'd do it just to do
> it, not to make the code cleaner, maybe it's just as good to put the
> prototypes by the functions.

In the case of that function you need a proto outside the file anyway.

Having to put a proto next to a function makes the whole thing seem
kind of pointless :-)

ron

[Myles Watson]

On Tue, Oct 27, 2009 at 10:10 AM, ron minnich  wrote:
> On Tue, Oct 27, 2009 at 8:17 AM, Myles Watson  wrote:
> > Something like that would be fine with me, but since we'd do it just to
>
> do
> > it, not to make the code cleaner, maybe it's just as good to put the
> > prototypes by the functions.
>
> In the case of that function you need a proto outside the file anyway.
>
> Having to put a proto next to a function makes the whole thing seem
> kind of pointless :-)

Agreed.  I don't like the missing prototype warning.

Myles

[Stefan Reinauer]

Myles Watson wrote:
> Maciej,
>
> Thanks for the patch.  I think most of it is ready to be committed.
> Index: src/lib/clog2.c
>
> --- src/lib/clog2.c	(revision 4869)
> +++ src/lib/clog2.c	(working copy)
> @@ -7,6 +7,8 @@
>  /* Assume 8 bits per byte */
>  #define CHAR_BIT 8
>
> +unsigned long log2(unsigned long x);
>
> unsigned long log2(unsigned long x)
>  {
>          // assume 8 bits per byte.
>
> Things like this make me wonder if we should just turn off the warning.  Is
> there a header file where we can put some of these prototypes?

No, we should not turn off the warning. It's the "Is our API correct?"
warning. We had very ugly bugs because of different prototypes in
different .c files.

Yes, there are many header files,.. check src/include for a fitting one.
If there is none, we should think about creating one.

[Myles Watson]

> -----Original Message-----
> From: Stefan Reinauer [mailto:[e-mail]]
> Sent: Wednesday, October 28, 2009 4:25 AM
> To: Myles Watson
> Cc: 'Maciej Pijanka'; [e-mail]
> Subject: Re: [coreboot] small patch for prototypes and unused variables
>
> Myles Watson wrote:
> > Maciej,
> >
> > Thanks for the patch.  I think most of it is ready to be committed.
> > Index: src/lib/clog2.c
> >
> > --- src/lib/clog2.c	(revision 4869)
> > +++ src/lib/clog2.c	(working copy)
> > @@ -7,6 +7,8 @@
> >  /* Assume 8 bits per byte */
> >  #define CHAR_BIT 8
> >
> > +unsigned long log2(unsigned long x);
> >
> > unsigned long log2(unsigned long x)
> >  {
> >          // assume 8 bits per byte.
> >
> > Things like this make me wonder if we should just turn off the warning.
>
> Is
> > there a header file where we can put some of these prototypes?
>
> No, we should not turn off the warning. It's the "Is our API correct?"
> warning. We had very ugly bugs because of different prototypes in
> different .c files.

In that case we ought to squash them all.  Too many warnings makes it hard
to see important ones.
> Yes, there are many header files,.. check src/include for a fitting one.
> If there is none, we should think about creating one.

Maybe we should create src/include/lib.h for all of the missing ones from
src/lib?

Thanks,
Myles

[Stefan Reinauer]

Myles Watson wrote:
> > -----Original Message-----
> > From: Stefan Reinauer [mailto:[e-mail]]
> > Sent: Wednesday, October 28, 2009 4:25 AM
> > To: Myles Watson
> > Cc: 'Maciej Pijanka'; [e-mail]
> > Subject: Re: [coreboot] small patch for prototypes and unused variables
> >
> > Myles Watson wrote:
> > > Maciej,
> > >
> > > Thanks for the patch.  I think most of it is ready to be committed.
> > > Index: src/lib/clog2.c
> > >
> > > --- src/lib/clog2.c	(revision 4869)
> > > +++ src/lib/clog2.c	(working copy)
> > > @@ -7,6 +7,8 @@
> > >  /* Assume 8 bits per byte */
> > >  #define CHAR_BIT 8
> > >
> > > +unsigned long log2(unsigned long x);
> > >
> > > unsigned long log2(unsigned long x)
> > >  {
> > >          // assume 8 bits per byte.
> > >
> > > Things like this make me wonder if we should just turn off the warning.
> >
> > Is
> > > there a header file where we can put some of these prototypes?
> >
> > No, we should not turn off the warning. It's the "Is our API correct?"
> > warning. We had very ugly bugs because of different prototypes in
> > different .c files.
>
> In that case we ought to squash them all.  Too many warnings makes it hard
> to see important ones.

Agreed. The old build system made it far to easy to ignore warnings. Now
with Kconfig we have a serious chance of detecting issues with our code. :-)

I think this is very healthy.
> > Yes, there are many header files,.. check src/include for a fitting one.
> > If there is none, we should think about creating one.
>
> Maybe we should create src/include/lib.h for all of the missing ones from
> src/lib?

Stuff like sprintf would nicely fit in stdio or string.h, so the include
file hints in the man pages for those functions kind of fit. Maybe
that's odd taste, I don't know.

All the other stuff that does not implement libc functionality (or
explicitly deserves an own include file for some reason) should go into
a lib.h.

Maybe even just having lib.h is enough.