|
At least is doesn't do anything harmful 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
|
You just have to see the upside.
As long as they are sitting at keyboards,
they aren't operating heavy machinery or driving a vehicle on the same roads as you or your loved ones.
-Rd
|
|
|
|
|
5! I love an optimist! 
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
But they may be writing the code controlling the machinery 
|
|
|
|
|
Sadly true.
At least someone corrected the error in a later post.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
I almost choked. You have to pay me a new keyboard
|
|
|
|
|
Also, as a thumb rule, everything derived from object might be a class.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'.
I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
|
|
|
|
|
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;
public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };
void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};
T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}
T* __fastcall operator ->()
{
return &data;
}
__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};
The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}
It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately.
I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?
|
|
|
|
|
I think you're right, the Protector isn't protecting much, however can you relate the occasional deadlocks you're looking for to any of this code?
|
|
|
|
|
I suspect it is a deadlock based on the error report: "The program is freezing once a week". I have no hard evidence for this class beeing the cause of the problem except it's the only place in the program actually locking anything. I don't have more than that so i'm just looking for code that could potentially trigger a deadlock. (i. e. Unnecessary locks beeing acquired) If i'm lucky this was indeed the problem, if not there is at least fewer overhead if i remove the useless locks.
|
|
|
|
|
I think the problem is elsewhere. Changing this code will have some influence on the program behavior, if anything it most likely will reduce the frequency of deadlocks, making it even harder to pinpoint the real problem. I would not modify the code, except for adding observability (say logging to a file) until the deadlock is better diagnosed, then fix it and clean up whatever deserves cleaning up, but no sooner for those parts that may have overall influence.
|
|
|
|
|
If you mean that i should'nt use Shotgun debugging to counter Cargo cult programming you are probably right. (Who is inventing all those terms?) But removing unnecessary code this is something i would consider refacturing and i don't to intend to remove the protector class entirely, just the unnecessary locks.
|
|
|
|
|
iberg wrote: Who is inventing all those terms?
I'm not.
iberg wrote: just the unnecessary locks
yeah, they may heavily influence code execution times, and that is why I would not touch them unless I have a work hypothesis that links the problem to them. I'd rather improve observability (add logging) and do whatever it takes to increase the problem frequency (as in reduce unnecessary delays, replace human/external input by automatic/local input, reduce the data set). I rather get 10 crashes/deadlocks a day with lots of logging, than reduce the problem frequency to once a month, which makes it almost impossible to solve at all.
|
|
|
|
|
I'm not sure if it's cargo cult programming (based on no understanding, not even believed to be understood by the programmer), but it certainly seems you're right. The very same error is often seen in .NET with properties supposedly providing "thread-safe" access to objects, e.g.
public Person Owner
{
get { lock (this) { return _owner; } }
}
The lock of course adds only overhead, no thread safety.
|
|
|
|
|
Good to see C# is equally vulnerable. I thought of cargo cult programming as the ritual inclusion of code or program structures that serve no real purpose. Locking an algorithm against changes of it's variables is perfectly valid but in this app no external algorithm ever tried to acquire the critical section. (The original code contains public accessors for the lock, i removed them to check if they are used at all. Turned out they were unused...)
|
|
|
|
|
Maybe the author did have intention of using reference:
T __fastcall operator *()
{
EnterCriticalSection(&CS);
T value = data;
LeaveCriticalSection(&CS);
return value;
}
|
|
|
|
|
LOL, some people think randomly sprinkling locks into the code will make it thread safe
|
|
|
|
|
Freekin' maintenance coding...
foreach (string item in items)
{
server.Remove(item);
}
|
|
|
|
|
I thought everybody knew compilers can read comments and act accordingly... 
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
Probably meant to go back and check for it's existence in the server collection prior to attempting to remove it...
This is why you use //TODO: instead of just generic comments so you can find them all before release!
|
|
|
|
|
I use an "int __todo_description;" instead. Gives me a compiler warning. Now I only need to get rid of these 205 warnings 
|
|
|
|
|
easier:
#warning super cali fragilistic expi ali docious
A train station is where the train stops. A bus station is where the bus stops. On my desk, I have a work station....
_________________________________________________________
My programs never have bugs, they just develop random features.
|
|
|
|
|
Super Lloyd wrote: #warning super cali fragilistic expi ali docious
If my code would be in C#, I would use the #warning pragma 
int __todo; is the only way I've found that works between different C++ compilers.
|
|
|
|
|
It's a subtle hint... It really means "I won't fix this until I receive my check"
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
