Can someone explain what is happening here?

[duragezic]

I ran across some code that concerned me and want to understand why it works. Basically it went like this...

/* myFile.c */
int number0 = 0;
int number1 = 1;
int number2 = 2;
int number3 = 3;

typedef struct
{
void *addr;
int count;
} myTable_T;

const myTable_T configTable[4] = {
{ &number0, 10 },
{ &number1, 20 },
( &number2, 30 },
{ &number3, 40 }
};

void myRoutine()
{
const myTable_T *pTbl;
int j;

for (j = 0; j < 4; j++)
{
pTbl = configTable[j].addr;
if ((int) pTbl->addr != 0)
{
// do something
}
}


The lines that concern me are where pTbl is assigned, and then used in the condition. First, pTbl is a pointer to a struct myTable_T, but the right hand assignment is a void pointer. Then the next line does treat it as a struct still, as if struct member "addr" is actually another struct (it's not). How does that even work? It does function correctly. Also, even if the assignment to pTbl does actually return the struct pointer, doesn't the int cast actually cast away the void pointer? Because we need two dereferences, one for the pointer to the struct, one for the pointer to member addr.

I think at the least that pTbl can't access the second member in the struct. Later in the code, when the second member (count) is accessed, it's done directly from configTable, rather than through pointer pTbl. I think they tried to access it with pTbl but couldn't, so just went at it directly!

I'd expect to see those two lines something like:

pTbl = &configTable[j];
if (*((int *)pTbl->address) != 0)


Since configTable is an array of structs, I'd want each iteration to have pTbl point to the entire struct from array element. Then with a pointer to that struct, I'd use the "->" notation to dereference that pointer, access member "address", which is also a pointer, so I'll cast it to be good then derefence it to get the integer value it is pointing to. That way, all data types are correct, the intention I believe is easy to understand, and no warnings from compiler or an error checker tool (for example Misra rule violations). I mean, is there some reason to do it like that? I don't mean to tell other more senior developers how to write their code (i.e. "you didn't write it like I would!") but this way seems like it could be undefined behavior, or at least bad practice.

Like I said, it works somehow. Hope this isn't too hard to follow. It's really easy to see in the actual code.

 

[Cogman]

this will work, but is horrible code based on assumptions that really shouldn't be made, ever. basically, pTbl is getting the address of number0. pointers generally are 4 bytes wide, so when the check is made "if ((int) pTbl->addr != 0) " essentially what is being returned is the value of the number in the list.

So, for example, when j = 0, pTabl gets the &number0. When that is cast as a struct, the first 4 bytes will contain the data in number0, but the next four bytes will contain the data in number1.

Again, that's assuming the compiler didn't decide to store number0 completely in a register.

Whoever wrote this code should be shot.

 

[Ken g6]

Oh, that is ugly! :Q:disgust: I'm late to the party, but maybe I can elaborate a little.

OK, first, I'm not sure what's going on with "i" vs. "j", but you didn't include all the code. That said, I'll focus on configTable[0].

So, a struct is just a way of spelling out the types in a block of memory. (void*)configTable[0] is just the same as configTable[0].addr; configTable[0].count is like (int)(*(((char*)(&configTable[0]))+sizeof(void*))), the (char*) being there to make the pointer arithmetic use bytes.

OK, so pTbl = configTable[j].addr = &number0. I guess the use of void* avoids any type conversion warnings, or maybe they just ignored them. So *pTbl is in the same memory location as number0. Then, as above, pTbl->addr == (void*)(*ptbl) == (void*)number0. Because it's the first element in the struct, this might even work on 64-bit, where an int is still 32-bits, but a pointer is not; though that would throw out even more warnings.

Edit: cleanup/P.S. This should be a DailyWTF.

 

[seemingly random]

I'm surprised-

pTbl = configTable[j].addr;

compiles without at least warnings if not errors. It looks like the compiler is taking liberties. The fact that it works is a fluke. Is count ever accessed and is it correct? It would be interesting to see an assembly language dump of the generated object code.

For a command line unit test prog, you could add-

printf("%x:%d\n", pTbl.addr, pTbl.count);

inside the j loop to check it.

For a compiler test, put the data in a separate file and the declarations in .h file and see what happens. Also, check if there is a way to increase the compiler warning level.

---

For the line-

if ((int) pTbl->addr != 0)

the (int) cast should be removed (the '!= 0' could be removed also). This will cause problems if compiled for a 64 bit pointer and 32 bit integer architecture. Very large pointers would be truncated resulting in random runtime errors.

---

Whose compiler?

 

[xtknight]

The type of pointer pTbl is myTable_T. He's using the first element of myTable_T and the "->" notation as a way of dereferencing the pointer to a different type, instead of just saying *pTbl. The void* allows him to just set the pTbl pointer to whatever. void* is a pointer, thus it's always the same size as a "long int", or "int" on 32-bit architecture. I agree, it's pretty strange.

Notice how pTbl->count actually gives you random, unallocated pieces of memory.

It makes endian assumptions, but it works with strict aliasing and max optimizations enabled. I don't even know why you would ever write something like that unless you were trying to confuse somebody. If you were smart enough to create some hack like that, you were smart enough to do it the right way.

But I do get this warning:

myfile.c: In function ?myRoutine?:
myfile.c:29: warning: cast from pointer to integer of different size

And works on 32-bit and 64-bit here. The pointer's longer (8 bits -- 64-bit) than the int, so whenever you access the first 4 bits, you're fine anyways.

 

[blahblah99]

Originally posted by: duragezic
I ran across some code that concerned me and want to understand why it works. Basically it went like this...

/* myFile.c */
int number0 = 0;
int number1 = 1;
int number2 = 2;
int number3 = 3;

typedef struct
{
void *addr;
int count;
} myTable_T;

const myTable_T configTable[4] = {
{ &number0, 10 },
{ &number1, 20 },
( &number2, 30 },
{ &number3, 40 }
};

void myRoutine()
{
const myTable_T *pTbl;
int j;

for (j = 0; j < 4; j++)
{
pTbl = configTable[j].addr;
if ((int) pTbl->addr != 0)
{
// do something
}
}


The lines that concern me are where pTbl is assigned, and then used in the condition. First, pTbl is a pointer to a struct myTable_T, but the right hand assignment is a void pointer. Then the next line does treat it as a struct still, as if struct member "addr" is actually another struct (it's not). How does that even work? It does function correctly. Also, even if the assignment to pTbl does actually return the struct pointer, doesn't the int cast actually cast away the void pointer? Because we need two dereferences, one for the pointer to the struct, one for the pointer to member addr.

I think at the least that pTbl can't access the second member in the struct. Later in the code, when the second member (count) is accessed, it's done directly from configTable, rather than through pointer pTbl. I think they tried to access it with pTbl but couldn't, so just went at it directly!

I'd expect to see those two lines something like:

pTbl = &configTable[j];
if (*((int *)pTbl->address) != 0)


Since configTable is an array of structs, I'd want each iteration to have pTbl point to the entire struct from array element. Then with a pointer to that struct, I'd use the "->" notation to dereference that pointer, access member "address", which is also a pointer, so I'll cast it to be good then derefence it to get the integer value it is pointing to. That way, all data types are correct, the intention I believe is easy to understand, and no warnings from compiler or an error checker tool (for example Misra rule violations). I mean, is there some reason to do it like that? I don't mean to tell other more senior developers how to write their code (i.e. "you didn't write it like I would!") but this way seems like it could be undefined behavior, or at least bad practice.

Like I said, it works somehow. Hope this isn't too hard to follow. It's really easy to see in the actual code.

That code is so horrible my head hurts just by looking at it.

 

[IHateMyJob2004]

Originally posted by: blahblah99

Originally posted by: duragezic
I ran across some code that concerned me and want to understand why it works. Basically it went like this...

/* myFile.c */
int number0 = 0;
int number1 = 1;
int number2 = 2;
int number3 = 3;

typedef struct
{
void *addr;
int count;
} myTable_T;

const myTable_T configTable[4] = {
{ &number0, 10 },
{ &number1, 20 },
( &number2, 30 },
{ &number3, 40 }
};

void myRoutine()
{
const myTable_T *pTbl;
int j;

for (j = 0; j < 4; j++)
{
pTbl = configTable[j].addr;
if ((int) pTbl->addr != 0)
{
// do something
}
}


The lines that concern me are where pTbl is assigned, and then used in the condition. First, pTbl is a pointer to a struct myTable_T, but the right hand assignment is a void pointer. Then the next line does treat it as a struct still, as if struct member "addr" is actually another struct (it's not). How does that even work? It does function correctly. Also, even if the assignment to pTbl does actually return the struct pointer, doesn't the int cast actually cast away the void pointer? Because we need two dereferences, one for the pointer to the struct, one for the pointer to member addr.

I think at the least that pTbl can't access the second member in the struct. Later in the code, when the second member (count) is accessed, it's done directly from configTable, rather than through pointer pTbl. I think they tried to access it with pTbl but couldn't, so just went at it directly!

I'd expect to see those two lines something like:

pTbl = &configTable[j];
if (*((int *)pTbl->address) != 0)


Since configTable is an array of structs, I'd want each iteration to have pTbl point to the entire struct from array element. Then with a pointer to that struct, I'd use the "->" notation to dereference that pointer, access member "address", which is also a pointer, so I'll cast it to be good then derefence it to get the integer value it is pointing to. That way, all data types are correct, the intention I believe is easy to understand, and no warnings from compiler or an error checker tool (for example Misra rule violations). I mean, is there some reason to do it like that? I don't mean to tell other more senior developers how to write their code (i.e. "you didn't write it like I would!") but this way seems like it could be undefined behavior, or at least bad practice.

Like I said, it works somehow. Hope this isn't too hard to follow. It's really easy to see in the actual code.

That code is so horrible my head hurts just by looking at it.

I've seen worse. Atleast we can read through it once and understand what it is doing. The only bad thing is that people still like to prefix pointers with p even though we have modern IDEs and are not using text editors anymore. I can understand prefixing things when all you have is notepad or vi, and it is 1995. But in 2010, these older developers need to be punched in the nose by younger developers.

 

[Ken g6]

Oh, no, not another Hungarian Notation holy war!

But I have to ask...do you have an IDE that identifies pointers separately from other variables, perhaps by color coding? If so, what is it and how does it do it?

 

[degibson]

Originally posted by: IHateMyJob2004
I've seen worse. Atleast we can read through it once and understand what it is doing. The only bad thing is that people still like to prefix pointers with p even though we have modern IDEs and are not using text editors anymore. I can understand prefixing things when all you have is notepad or vi, and it is 1995. But in 2010, these older developers need to be punched in the nose by younger developers.

Grumpiness knows no age!

EDIT: vi for life

 

[Madwand1]

Originally posted by: duragezic
How does that even work? It does function correctly.
...
is there some reason to do it like that? I don't mean to tell other more senior developers how to write their code (i.e. "you didn't write it like I would!") but this way seems like it could be undefined behavior, or at least bad practice.

Like I said, it works somehow. Hope this isn't too hard to follow. It's really easy to see in the actual code.

The calling code at least is pretty nutty, and I can't see a claim that it works being valid, because I can't guess what it's trying to do, and it doesn't get past my (C++) compiler.

The next idea that comes to mind is sending whoever wrote it back to school, accompanied of course by anyone who think that Hungarian is the problem with this code...

That aside, I think that the amount of time you've probably wasted on this code is a good enough justification to get whoever is responsible for it to explain it if not improve it.

 

[duragezic]

Thanks for all the explanations from everyone. I had an idea why it worked but I know I can count on you smart folks to learn that and more about it. As for the actual code, I asked a lead about it and they were just as :Q and /facepalm as some of you. And it is being fixed.

There have been other times where I've seen pieces of code and went "what" because it doesn't seem like it would work from my knowledge of C, which I admit is not that great. I do like to understand why it works, and question it to get it changed if it is that bad. But I usually see that more with tester code than the developers.

As for the p prefix for pointers I agree it isn't really necessary. However the project's standards require that. It's a fairly old project that is nearly over.

And for an IDE, I use SlickEdit at work. I just installed the newest version (2009) at work and one of its new features is color coding symbols differently although I'm not sure if it differentiates between pointers.