Tell me what's wrong with the following
-
In short: everything! Where should I start? The unsigned long of 1 bit? The SendBlock doubly defined? The version union with members in wrong order? The complete lack of comments? I hope they pay you well to put up with this pile of manure :wtf:
Mircea
The union with members in the wrong order is likely correct. In
Version1,EventTimewas added to timestamp messages. It's followed by theMessagecontents, which are at the end of the message and claim to be of typechar[1], but which will actually be indexed from0toLength - 1. Writing types for message byte buckets is fun, especially when you add protocol version control!Robust Services Core | Software Techniques for Lemmings | Articles
The fox knows many things, but the hedgehog knows one big thing. -
The union with members in the wrong order is likely correct. In
Version1,EventTimewas added to timestamp messages. It's followed by theMessagecontents, which are at the end of the message and claim to be of typechar[1], but which will actually be indexed from0toLength - 1. Writing types for message byte buckets is fun, especially when you add protocol version control!Robust Services Core | Software Techniques for Lemmings | Articles
The fox knows many things, but the hedgehog knows one big thing.Greg Utas wrote:
type char[1], but which will actually be indexed from 0 to Length - 1
My instincts are telling me that it's a placeholder for variable-length data. With MSVC you cannot use a zero-length array unless the following conditions are true: 1.) It must be the last field in the union or struct. 2.) Option /Za must be enabled. 3.) Option /permissive- must be unset/disabled Or you can just do char[1] :-D
-
Greg Utas wrote:
type char[1], but which will actually be indexed from 0 to Length - 1
My instincts are telling me that it's a placeholder for variable-length data. With MSVC you cannot use a zero-length array unless the following conditions are true: 1.) It must be the last field in the union or struct. 2.) Option /Za must be enabled. 3.) Option /permissive- must be unset/disabled Or you can just do char[1] :-D
And then you receive a zero-length message (header only), and someone does a
memsetusing the size of this type, trampling over thechar[1]that isn't there. :)Robust Services Core | Software Techniques for Lemmings | Articles
The fox knows many things, but the hedgehog knows one big thing. -
And then you receive a zero-length message (header only), and someone does a
memsetusing the size of this type, trampling over thechar[1]that isn't there. :)Robust Services Core | Software Techniques for Lemmings | Articles
The fox knows many things, but the hedgehog knows one big thing. -
struct Record { unsigned short Length; unsigned long ObjectId; LevelType Level; unsigned long SendBlock; struct { unsigned long SendBlock : 31; unsigned long SendBlockMarker : 1; }; \_\_time32\_t SendTime; struct { Type SendType : 16; unsigned SendVersion : 16; }; unsigned long SendNumber; union { struct { char Message\[1\]; } Version0; struct { \_\_int64 EventTime; char Message\[1\]; } Version1; }; };:face-palm:
Software Zen:
delete this;Gary R. Wheeler wrote:
struct { Type SendType : 16; unsigned SendVersion : 16; };
This appears to be a bit field shared between two different data types. I am assuming
Typeis an enumeration type which is a signed integer. The other 16 bits are unsigned. -
The two
SendBlockfields? Maybe VS2008 accepted it because the second one was inside an unnamedstruct.Robust Services Core | Software Techniques for Lemmings | Articles
The fox knows many things, but the hedgehog knows one big thing.BINGO!
Software Zen:
delete this; -
Greg Utas wrote:
type char[1], but which will actually be indexed from 0 to Length - 1
My instincts are telling me that it's a placeholder for variable-length data. With MSVC you cannot use a zero-length array unless the following conditions are true: 1.) It must be the last field in the union or struct. 2.) Option /Za must be enabled. 3.) Option /permissive- must be unset/disabled Or you can just do char[1] :-D
Randor wrote:
My instincts are telling me that it's a placeholder for variable-length data.
Exactly right. The
structis actually header for the information content which follows.Software Zen:
delete this; -
In short: everything! Where should I start? The unsigned long of 1 bit? The SendBlock doubly defined? The version union with members in wrong order? The complete lack of comments? I hope they pay you well to put up with this pile of manure :wtf:
Mircea
Mircea Neacsu wrote:
The unsigned long of 1 bit?
An
unsignedvalue 1 bit in width is slightly more useful than a signed value 1 bit in width, if you think about it.Mircea Neacsu wrote:
The SendBlock doubly defined?
That's the error I was referring to. IMO the compiler should issue an error, since the identifier
SendBlockis ambiguous when referencing fields in thestruct. This code is compiled using VS2008 which does not issue a diagnostic.Mircea Neacsu wrote:
The version union with members in wrong order?
They're in the correct order. The
Messagevalue must be at the end of the structure, since it is a placeholder for the beginning of the variable-length data that's included in the message.Mircea Neacsu wrote:
The complete lack of comments?
This is a code fragment, extracted from a header file that provides more context.
Mircea Neacsu wrote:
hope they pay you well to put up with this pile of manure :WTF:
They do pay me well, in the coin that really matters: appreciation. I've worked for the same company for over 30 years, and have survived numerous "workforce reduction" actions during the last period of financial instability. As to its fecal quality, I didn't create this originally but I have maintained it over the last 20 years. This 'weird & wonderful' came up during some refactoring I'm doing while adding a new feature.
Software Zen:
delete this; -
Gary R. Wheeler wrote:
struct { Type SendType : 16; unsigned SendVersion : 16; };
This appears to be a bit field shared between two different data types. I am assuming
Typeis an enumeration type which is a signed integer. The other 16 bits are unsigned.As someone else mentioned, this is part of a message protocol definition. This
structwas originally a single, unsigned 32-bit value calledSendTypewhich was indeed an enumeration. We've occasionally had the need to add features without removing the ability to handle older versions. This was one solution to the problem. Messages in the original format (which don't know aboutSendVersion) therefore have an appropriateSendTypewith aSendVersionof zero (0).Software Zen:
delete this; -
Mircea Neacsu wrote:
The unsigned long of 1 bit?
An
unsignedvalue 1 bit in width is slightly more useful than a signed value 1 bit in width, if you think about it.Mircea Neacsu wrote:
The SendBlock doubly defined?
That's the error I was referring to. IMO the compiler should issue an error, since the identifier
SendBlockis ambiguous when referencing fields in thestruct. This code is compiled using VS2008 which does not issue a diagnostic.Mircea Neacsu wrote:
The version union with members in wrong order?
They're in the correct order. The
Messagevalue must be at the end of the structure, since it is a placeholder for the beginning of the variable-length data that's included in the message.Mircea Neacsu wrote:
The complete lack of comments?
This is a code fragment, extracted from a header file that provides more context.
Mircea Neacsu wrote:
hope they pay you well to put up with this pile of manure :WTF:
They do pay me well, in the coin that really matters: appreciation. I've worked for the same company for over 30 years, and have survived numerous "workforce reduction" actions during the last period of financial instability. As to its fecal quality, I didn't create this originally but I have maintained it over the last 20 years. This 'weird & wonderful' came up during some refactoring I'm doing while adding a new feature.
Software Zen:
delete this;Gary R. Wheeler wrote:
An unsigned value 1 bit in width is slightly more useful than a signed value 1 bit in width, if you think about it.
Yes, but long... :confused:
Gary R. Wheeler wrote:
They do pay me well, in the coin that really matters: appreciation. I've worked for the same company for over 30 years,
I understand. I've worked for the same company for 22 years and enjoyed every minute of it... well, at least most minutes :laugh:
Mircea
-
Gary R. Wheeler wrote:
An unsigned value 1 bit in width is slightly more useful than a signed value 1 bit in width, if you think about it.
Yes, but long... :confused:
Gary R. Wheeler wrote:
They do pay me well, in the coin that really matters: appreciation. I've worked for the same company for over 30 years,
I understand. I've worked for the same company for 22 years and enjoyed every minute of it... well, at least most minutes :laugh:
Mircea
Mircea Neacsu wrote:
Yes, but long
In this environment, 32 bits. The original value was an
unsigned long.Software Zen:
delete this; -
As someone else mentioned, this is part of a message protocol definition. This
structwas originally a single, unsigned 32-bit value calledSendTypewhich was indeed an enumeration. We've occasionally had the need to add features without removing the ability to handle older versions. This was one solution to the problem. Messages in the original format (which don't know aboutSendVersion) therefore have an appropriateSendTypewith aSendVersionof zero (0).Software Zen:
delete this;This smells like nifty code. Nifty code is brittle. If it's difficult to understand a struct, you're doing it wrong. That said, you have existing code and must maintain compatibility so...
Charlie Gilley “They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759 Has never been more appropriate.
-
This smells like nifty code. Nifty code is brittle. If it's difficult to understand a struct, you're doing it wrong. That said, you have existing code and must maintain compatibility so...
Charlie Gilley “They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759 Has never been more appropriate.
charlieg wrote:
This smells like nifty code. Nifty code is brittle. If it's difficult to understand a struct, you're doing it wrong.
I heartily agree. This
structdefines the message format used by our in-house diagnostic tool. Our applications are multi-machine, multi-process, and multithreaded. Each process includes a small TCP/IP socket-based server that communicates with a client application. Code in the process uses the server likeprintfon steroids. The client in turn handles as many server connections as needed, displaying and recording them. The servers and this tool has been essential for both development and problem diagnosis in the field for us for the last 20 years. It's been maintained and refactored numerous times. We have systems in the field that are over 10 years old, so backward compatibility is an absolute requirement. The good news is the code around this definition abstracts away the awkwardness of this definition.Software Zen:
delete this; -
struct Record { unsigned short Length; unsigned long ObjectId; LevelType Level; unsigned long SendBlock; struct { unsigned long SendBlock : 31; unsigned long SendBlockMarker : 1; }; \_\_time32\_t SendTime; struct { Type SendType : 16; unsigned SendVersion : 16; }; unsigned long SendNumber; union { struct { char Message\[1\]; } Version0; struct { \_\_int64 EventTime; char Message\[1\]; } Version1; }; };:face-palm:
Software Zen:
delete this;And the structure is just evil completely through... If you have to stare at something like this - here's your sign.
Charlie Gilley “They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759 Has never been more appropriate.
-
struct Record { unsigned short Length; unsigned long ObjectId; LevelType Level; unsigned long SendBlock; struct { unsigned long SendBlock : 31; unsigned long SendBlockMarker : 1; }; \_\_time32\_t SendTime; struct { Type SendType : 16; unsigned SendVersion : 16; }; unsigned long SendNumber; union { struct { char Message\[1\]; } Version0; struct { \_\_int64 EventTime; char Message\[1\]; } Version1; }; };:face-palm:
Software Zen:
delete this;
