patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5822, Handle unused arguments

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5822: Handle unused arguments

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Mon 26 Mar 2007 04:03:29 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 24 Apr 2007 08:42:57 AM UTC, comment #14: 

Thanks, checked it in.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 08:24:25 AM UTC, comment #13: 

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)

Tai-hwa Liang <atliang>
Mon 16 Apr 2007 07:47:39 PM UTC, 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.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Apr 2007 10:21:22 PM UTC, comment #11: 

"-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.

Jonathan Larmour <jifl>
Group Member
Wed 11 Apr 2007 07:41:14 PM UTC, comment #10: 

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.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 04 Apr 2007 07:35:05 PM UTC, 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.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Mar 2007 07:55:51 AM UTC, comment #8: 

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.

Tai-hwa Liang <atliang>
Wed 28 Mar 2007 07:22:42 AM UTC, comment #7: 

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...

Frédéric Bernon <fbernon>
Group Member
Wed 28 Mar 2007 07:00:10 AM UTC, comment #6: 

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 :)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Mar 2007 02:12:41 AM UTC, comment #5: 

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)

Tai-hwa Liang <atliang>
Tue 27 Mar 2007 07:45:38 AM UTC, comment #4: 

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.

Kieran Mansley <kieranm>
Group Member
Mon 26 Mar 2007 04:22:02 PM UTC, comment #3: 

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?

Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 04:17:36 PM UTC, comment #2: 

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.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Mar 2007 04:07:29 PM UTC, comment #1: 

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 ?


Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 04:03:29 PM UTC, original submission:  

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.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12584:  unused.patch added by atliang (1KiB - application/octet-stream)
file #12314:  unused.patch added by atliang (4KiB - application/octet-stream - LWIP_UNUSED_ARG patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by atliang (Updated the item)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-24 atliang Attached File- Added unused.patch, #12584
    2007-04-16 goldsimon StatusIn Progress Done
        Open/ClosedOpen Closed
    2007-04-04 goldsimon Assigned toNone goldsimon
    2007-04-04 goldsimon StatusNone In Progress
    2007-03-28 atliang Attached File- Added unused.patch, #12314

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code