|
What's wrong isn't obvious to me. 
It looks like SendVersion tells you which union to look at.
If sent interprocessor, it will pack differently on 32-bit and 64-bit CPUs, ignoring endianism problems. The fact that there's a version number suggests that this could be the case.
|
|
|
|
|
There's a #pragma pack around the declaration.
I'll give you a couple hints: it's before SendVersion. This compiles successfully under VS2008 (don't ask), and I've not tried compiling it under VS2019. It should trigger a compile error.
Software Zen: delete this;
|
|
|
|
|
The two SendBlock fields? Maybe VS2008 accepted it because the second one was inside an unnamed struct.
|
|
|
|
|
BINGO!
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 
Mircea
|
|
|
|
|
The union with members in the wrong order is likely correct. In Version1, EventTime was added to timestamp messages. It's followed by the Message contents, which are at the end of the message and claim to be of type char[1], but which will actually be indexed from 0 to Length - 1.
Writing types for message byte buckets is fun, especially when you add protocol version control!
|
|
|
|
|
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] 
|
|
|
|
|
And then you receive a zero-length message (header only), and someone does a memset using the size of this type, trampling over the char[1] that isn't there. 
|
|
|
|
|
Heh,
I'm not advocating the technique, I am just pointing out what I see in the struct.
|
|
|
|
|
Randor wrote: My instincts are telling me that it's a placeholder for variable-length data.
Exactly right. The struct is actually header for the information content which follows.
Software Zen: delete this;
|
|
|
|
|
Mircea Neacsu wrote: The unsigned long of 1 bit?
An unsigned value 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 SendBlock is ambiguous when referencing fields in the struct. 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 Message value 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
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... 
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 
Mircea
|
|
|
|
|
Mircea Neacsu wrote: Yes, but long
In this environment, 32 bits. The original value was an unsigned long.
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 Type is 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 struct was originally a single, unsigned 32-bit value called SendType which 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 about SendVersion) therefore have an appropriate SendType with a SendVersion of 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.
|
|
|
|
|
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 struct defines 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 like printf on 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;
|
|
|
|
|
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.
|
|
|
|
|
Message would cause a name clash in the 2 struts.
So MessageOld, MessageNew
|
|
|
|
|
In a standup today, discussing a particular code repository. Sometime in the past, someone created a backup of one of the projects, then worked on the original project. Afterwards they checked everything back into repository, including the backup.
So my team is now working on this, and the developer was wondering whether this backup needs to be updated. I commented that if this is just a backup and no longer being used, we should just delete it. Keeping in mind that if it is still needed, it still exists in the history of the git repo.
So of course a big discussion began, with the other developer arguing that we don't know why it is still there, maybe it's still needed, etc. The final decision was to take this one instance to the main IT manager to provide "guidance".
Whatever happened to developers taking responsibility and making informed decisions? We are supposed to be senior developers, and capable of making a decision without having to have a committee meeting, or passing the buck to senior management.
If I was that manager and was asked that question, I think I would be a bit pissed off and have some serious doubts about this team....
EDIT
I should have remembered, this is a large government agency. And there is a huge mentality of covering you b#?! and taking responsibility for anything. I guess that is what they have us contractors for....
|
|
|
|
|
Is it causing any trouble?
|
|
|
|
|
Yes and no. Personally I don't care that much one way or another. I just find that this is a lot of passing the buck, and a non-willingness to clean up things that are not needed (a bit of YAGNI I feel).
However we do have automated scans that look at the repos and see which might need work, such as updating libraries and. Net versions used. Keeping this in there just generates false positives that then have to be looked at. And these projects tend to be floated around various teams that may not be aware of what these duplicated projects are about.
I am of the mindset that if I am working on a project, and find unused code, it should be cleaned up. Just more technical debt to deal with.
|
|
|
|
|
Yeah, I see that.
But, as you say, even deleted, it's still there. So I really don't see the point.
Where I am, we have an issue where there are database tables which are no longer in use.
I want to leave them there. That's what I do.
The DBA says we should free up the disk space they use.
I said we can TRUNCATE them to free up the space, but that DROPping them isn't going to make a difference, and that they can always be DROPped later.
So they said OK, who'll do it?
I said I'd write a script.
I wrote a script, ran it on the DEV database, it was fine.
But no one will man-up and run it on UAT or PROD -- I can't, I'm the developer.
I think my script has been sitting there for a year now. :shrug:
|
|
|
|
|
hardware is cheap, data is priceless.
As long as you document the tables no longer in use and WHY, for Jesus', Joseph's, and Mary's sake - document why! (sorry, I miss my dad. When I really did something stupid his go to profanity was JJ&M )
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.
|
|
|
|
|
If you're working with a government agency I think you'll find this transcends fields.
The attitude of working lights on at 9 lights off at 5, and not caring more than you're being paid to care is a foreign concept to many of us working in software development, but it is de rigueur in government, at least in the states.
I think part of it is the methodical yet inflexible way they do performance reviews. You simply have to check all the boxes, and you don't really get rewarded for going "above and beyond" - you get rewarded for seniority, which leads to a lot of people simply keeping their heads down.
I don't think it's all a bad thing. A lot of private sector jobs have a culture that "rewards extra effort" but what it eventually devolves to (if it ever was anything else) is a culture of overworking employees. That's almost impossible to do when you don't reward going above and beyond.
To err is human. Fortune favors the monsters.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
