patchlwIP - A Lightweight TCP/IP stack - Patches: patch #7130, mib2.c: remove meaningless const...

 
 

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

patch #7130: mib2.c: remove meaningless const qualifiers

Submitter:  Luca Ceresoli <lucaceresoli>
Submitted:  Tue 23 Mar 2010 04:19:46 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

Sat 27 Mar 2010 04:25:47 PM UTC, comment #19: 

Checked in.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 27 Mar 2010 09:23:34 AM UTC, comment #18: 

Oh, OK. The const in the cast is unnecessary (and therefore not supported by some compilers, or cdecl?) as it is meaningless because the value of the pointer is copied into the array. Somehow, I didn't get that before ;-)

Being like that, it seems it's indeed safe to apply your patch as it doesn't change anything. I'll apply it.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 05:18:41 PM UTC, comment #17: 

Sorry for lighting such a fire...

My understanding is that

> struct mib_node* const snmp_nodes[28]

declares an array of immutable pointers, hence repeating the constness for each member is redundant (not useful, not hurting).
I cannot explain why my compiler is so hurt by that, though.

The intent of my patch was to remove a deliberately useless specifier to clean up code and (in my case) remove warnings.

My picky compiler's help page about this warning follows.

--------------8<------------------

Compiler Warning: type qualifier is meaningless on cast type

Description:
The compiler has encountered casting to a qualified type. The statement is valid, but pointless.

Severity:
Warning

Recovery:
After the warning, compilation continues.

Example:
int main()
{
int Num = 1;
int Num_c = (const)Num;
}

How to Fix:
The statement is pointless. Therefore, change the source to ensure that removing the statement will have no ill effect on the program.

Related Information:
ISO/IEC 9899:1990 "Programming Languages - C"
ISO/IEC 14882:1998 "Programming Languages - C++" (5.2.7, 5.2.9, 5.2.10)

--------------8<------------------

Luca Ceresoli <lucaceresoli>
Fri 26 Mar 2010 02:14:57 PM UTC, comment #16: 


> [..] but I wouldn't have done it this way.


That thought comes to my mind often when looking at the snmp code ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 02:01:06 PM UTC, comment #15: 

It's not a syntax error - it's a semantic error that some compilers have decided to warn about an unnecessary pointer qualifier.  Just like some compilers say the & on a function is not needed and others don't.

This construct (initializer) is NOT portable.  There should be a struct with the common area up front and a union of structs to make the nodes different based on that MB_NODE_xxx goodie.  This probably works because the structs start out the same and are pointers, but I wouldn't have done it this way.

I don't insist anyone change it either :) Unless someone shows up and says it won't work this way on their platform.

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 01:54:32 PM UTC, comment #14: 

BTW: I don't insist on changing this, but there seems to be something wrong with it or Luca's compiler wouldn't warn and "cdecl" wouldn't say it's a syntax error.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 01:53:05 PM UTC, comment #13: 

The problem with no casts in the array initializer is that the array elements have a different type than the array itself.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 01:47:44 PM UTC, comment #12: 

Here's what I agree to: :-)

If snmp_nodes will not be changed at any time:

struct mib_node const const snmp_nodes[28] = { (const struct mib_node)&snmp_scalar, ...

If snmp_nodes will be changed then:

struct mib_node const snmp_nodes[28] = { (const struct mib_node)&snmp_scalar, ...

The pointers must be to const because they are initialized to const objects.  If this is done corretly, there should be NO casts required on the array initializers.

So probably:

struct mib_node const * const snmp_nodes[28] = { &snmp_scalar, ...

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 01:43:10 PM UTC, comment #11: 

Except that the thing it points to IS const...

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 01:42:27 PM UTC, comment #10: 

grr, asterisks in previous comment seem to have been interpreted as making things bold.  Sounds like we're all in agreement though about the meaning of current code, but we're not sure why it was written that way.

Kieran Mansley <kieranm>
Group Member
Fri 26 Mar 2010 01:40:21 PM UTC, comment #9: 

Posted too fast: maybe it's not so weird.  I think it's trying to say that the pointer is a const, rather than the thing it points to being a const. i.e. if you have

const char * foo;
char * const bar;

You can modify foo but not *foo, and you can modify *bar but not bar.

To complete Bill's list:

const struct mib_node* snmp_nodes[28] is an array of pointers to const struct mib_node.

(const struct mib_node*)&snmp_scalar is a pointer to a const struct mib_node.

(struct mib_node* const)&snmp_scalar is a const (i.e. unmodifiable) pointer to a struct mib_node


In summary, I'm not sure what's wrong with the current code, assuming that they meant to declare const pointers rather than pointers to consts.

Kieran Mansley <kieranm>
Group Member
Fri 26 Mar 2010 01:39:28 PM UTC, comment #8: 

This is what cdecl has to say about the two versions:

a) (current code)
cdecl explain "struct mib_node* const snmp_nodes[28]"
declare snmp_nodes as array 28 of const pointer to struct mib_node

b)
cdecl explain "const struct mib_node* snmp_nodes[28]"
declare snmp_nodes as array 28 of pointer to const struct mib_node

So a) means the pointers are const while b) means the actual structs are const. However, in the code, I don't see why the pointers should be const but the structs should not.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 01:38:56 PM UTC, comment #7: 

Ok, I see.  I was showing (initially) why the original const had a warning and thought the intent was the pointer was to const, and it should be.

You want:

struct mib_node const * const snmp_nodes[28] = {
(const struct mib_node*)&snmp_scalar, ...

because snmp_scalar is const.  If snmp_nodes[] can change, then the second const is wrong.  I see no references to snmp_nodes though.

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 01:30:18 PM UTC, comment #6: 

The current code is weird then.  I'm not sure what struct mib_node* const snmp_nodes[28] is supposed to mean either.

Kieran Mansley <kieranm>
Group Member
Fri 26 Mar 2010 01:20:37 PM UTC, comment #5: 


> const struct mib_node* snmp_nodes[28] is an array of pointers to
> const struct mib_node.


Yes, but the current code is

struct mib_node* const snmp_nodes[28] = {

not

const struct mib_node* snmp_nodes[28] = {

as you wrote.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 12:55:39 PM UTC, comment #4: 


>But that means something different than:
>"struct mib_node* const snmp_nodes[28]"


It doesn't mean something differet.

const struct mib_node* snmp_nodes[28] is an array of pointers to const struct mib_node.

(const struct mib_node*)&snmp_scalar is a pointer to a const struct mib_node.

(struct mib_node* const)&snmp_scalar is a pointer to a struct mib_node (the const modifies nothing, hence the warning).

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 12:48:20 PM UTC, comment #3: 

But that means something different than:

"struct mib_node* const snmp_nodes[28]"

which the casted pointer is assigned to. But maybe that's what it was meant to be:

const struct mib_node* snmp_nodes[28] = {
  (const struct mib_node*)&snmp_scalar, (const struct mib_node*)&snmp_scalar,

but that doesn't compile (warning: "different 'const'-identifier"). Although to me, that should mean "snmp_nodes is an array of 28 pointers to const struct mib_node's" (pointer may be modified, actual variable not), whereas the original definition seems to lack the "actual variable is const"...?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 26 Mar 2010 12:31:10 PM UTC, comment #2: 

const is allowed in a cast, but it matters where:

(const struct mib_node*)&snmp_scalar

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 10:34:25 AM UTC, comment #1: 

Hmm, I don't get any warning with or without the "const"s removed by the patch. Also, I didn't find any documentation on whether that "const" is allowed in casts (it is allowed in declarations, so why not in casts?), and the "cdecl" tool tells me this is a "syntax error"...

So the question is, is it really safe to remove these consts?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 23 Mar 2010 04:19:46 PM UTC, original submission:  

While compiling mib3.c, my compiler issues a lot of

> warning: type qualifier is meaningless on cast type


The attached patch removes them.

Luca Ceresoli <lucaceresoli>

 

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

Attached Files
file #20007:  mib2-const.patch added by lucaceresoli (19KiB - text/x-patch - patch to remove meaningless const qualifiers)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by lucaceresoli (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-03-27 goldsimon StatusNeed Info Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2010-03-26 goldsimon StatusNone Need Info
    2010-03-23 lucaceresoli Attached File- Added mib2-const.patch, #20007

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code