[patch #5822] Handle unused arguments

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

URL:
  <http://savannah.nongnu.org/patch/?5822>

                 Summary: Handle unused arguments
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: goldsimon
            Submitted on: Montag 26.03.2007 um 16:03
                Category: None
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

In some functions unused arguments are used (e.g. "arg;" or "arg = arg;") to
prevent compiler warnings. This should either be modified to something like
"LWIP_UNUSED_ARG(arg);" or completely left away.




    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #1, patch #5822 (project lwip):

Ok for me, but can you explain what is the problem? Do you think a binary
code is generated by such instructions ? Or is it just for source code style?
How do you want to define LWIP_UNUSED_ARG ?



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #2, patch #5822 (project lwip):

It's not that I fear these statements would bloat lwIP binary size, only that
different compilers have very different views of what to warn about.
Statements like "arg = arg;" or "arg;" often give a warning like "statement
has no effect". Personally, I prefer simply not to warn about unused
arguments.
I would define LWIP_UNUSED_ARG to nothing by default and let people override
it if they really need it.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #3, patch #5822 (project lwip):

Ok I understand. In my case, my compiler generate warning to any unsed
arguments. Your solution can help to avoid warning to everyone. I suppose
cc.h is the good location to define it, right?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #4, patch #5822 (project lwip):

The best solution would be to remove any unused arguments from the function
declaration!

However, I agree that the LWIP_UNUSED_ARG macro would be a step in the right
direction.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #5, patch #5822 (project lwip):

Not sure about where is the right place to put LWIP_UNUSED_ARG as cc.h is
port specific.  For now I leave it in lwip/arch.h.

I've tested the attached patch with Paradigm C++ Pro 6 and gcc 4.1.2(with
-Wunused -Wunused-parameter).  Feedbacks are welcomed.

(file #12314)
    _______________________________________________________

Additional Item Attachment:

File name: unused.patch                   Size:4 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #6, patch #5822 (project lwip):

That patch seems rather complete, but I want to look over all the code again:
as Kieran said: The best solution would be to remove any unused arguments from
the function declaration!

In my opinion, cc.h is a good location since it _is_ port specific. Some
compilers complain like "statement has no effect" when they find something
like "(void)arg;" (although Paradigm and gcc don't seem among them, at least
with your compile time options :)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #7, patch #5822 (project lwip):

About remove any unsed arguments, it will not be possible always. By example,
in api_msg.c, in recv_udp, the prototype is defined by udp_recv, so, you can't
change it without breaking the api.

Why not about arch.h? It already give default define for PACK_STRUCT_BEGIN
and others, and it also include cc.h. So, if this default is not good for
your compiler, you can provide your own define in your cc.h (in fact, it's
more compiler than port dependant).

This default is good for me, if it's good for gcc.

#ifndef LWIP_UNUSED_ARG
#define LWIP_UNUSED_ARG(x) (void)x
#endif /* LWIP_UNUSED_ARG */

I think your can integrate that...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #8, patch #5822 (project lwip):

Forgot to mention that my patch was tested with all "Inefficient Coding"
warnings turned on in Paradigm Pro, particularly, "Parameter 'ident is never
used" and "'ident declared but never used."

Regarding to unused argument removal, I think that's not that easy without
breaking existing APIs. For example, ip_timer() utilises sys_timeout()
interface, which requires a timeout handler with void argument.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Update of patch #5822 (project lwip):

                  Status:                    None => In Progress            

    _______________________________________________________

Follow-up Comment #9:

I'm including

#ifndef LWIP_UNUSED_ARG
#define LWIP_UNUSED_ARG(x) (void)x
#endif /* LWIP_UNUSED_ARG */

into arch.h and starting to update the stack to use this new define where
necessary. Everyone, please update to LWIP_UNUSED_ARG if I miss something.
Thanks.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Update of patch #5822 (project lwip):

             Assigned to:                    None => goldsimon              


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #10, patch #5822 (project lwip):

I've converted all statements like '(void)arg;' into 'LWIP_UNUSED_ARG(arg);'

Does somebody know how to get gcc to generate a warning for statments like
'if(arg);' or 'arg = arg;'??? That would simplify getting all the points
where LWIP_UNUSED_ARG() should be used.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #11, patch #5822 (project lwip):

"-Wuninitialized -Winit-self" should do all the 'arg = arg' cases.

For 'if(arg);' Supposedly -Wextra warns when:
' An empty body occurs in an `if' or `else' statement.'

It also warns about a lot of other things too so may well be quite noisy. I
guess you can search in the compiler output for the specific warning message
that gets produced for that case.

BTW, I hadn't noticed this patch discussion before. Out of interest, in our
code we use the following which from experience on various compilers is more
portable at preventing warnings - some compilers are quite aggressive at
reporting them. It's slightly harder to apply retrospectively because you
need the type though:

#define CYG_UNUSED_PARAM( _type_, _name_ ) do {                 \
  _type_ __tmp1 = (_name_);                                     \
  _type_ __tmp2 = __tmp1;                                       \
  __tmp1 = __tmp2;                                              \
while(0)

I imagine it's not worth changing now you've already been through the code.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Update of patch #5822 (project lwip):

                  Status:             In Progress => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #12:

re comment #11:

Furtunately, gcc does not warn if you cast the unused variable to (void).
Either most of the users use gcc or many other compilers do it like this (or
don't warn at all?). But casting to void (as it is now) was in the code
already. This 'patch' only moves the casting into a define, which gives us a)
cleaner code and b) the possibility to adopt to other compilers.
Unfortunately, type information is not included...

The downside to your approach is that the statement (if not optimized away)
generates code, while '(void)arg;' doesn't (even with '-O0' for gcc).

If you want to change it, though, the places to change are now easy to find
;-)

Closing this as done, as I didn't find more appearences.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #13, patch #5822 (project lwip):

Just ran into another three unused parameter in LWIP_RAW=1, LWIP_REASSEMBLY=1
and LWIP_ARP=1 cases.  See attached patch for quick a fix.

(file #12584)
    _______________________________________________________

Additional Item Attachment:

File name: unused.patch                   Size:1 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5822] Handle unused arguments

Ashley Duncan

Follow-up Comment #14, patch #5822 (project lwip):

Thanks, checked it in.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel