18 replies to this topic

[xorangekiller]

Dr_Asik, on 10 September 2012 - 02:39, said:

By the way, the comments are the least of your worries about the last function you posted. In order of importance:

1)

  while( this->Plugins.back().File != NULL ) Sleep( 1000 ); // Wait until the thread is done loading the plugin.
  Sleep( 500 ); // Wait for another half-second, just to be sure the thread has had sufficient time to cleanup.

Waiting half a second doesn't guarantee any particular thread will run at all. This might come up 1 time out of 10000 in particular conditions which you might not even be able to reproduce. You should be using a proper thread synchronization mechanism such as a monitor here.

2)

  this->Plugins.back().Provider = new EVPlugin( this->ProviderBase, 0, this->ED, this->Tabs );

Any "new" operator in methods is a red flag. Exceptions can be thrown anywhere (including by the "new" operator itself) and prevent any cleanup code from executing, leading to memory leaks. Plus since there's no corresponding "delete" within the method, the cleanup responsibility is apparently held by some different code, which makes your memory management difficult to reason about (and probably wrong). The only way to guarantee proper cleanup in C++ is RAII, use it.

3) Some stylistic issues:
- Inconsistent use of C/C++-style casts:

CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE) &PluginThreadProc, reinterpret_cast< void * >( &this->Plugins.back() ), 0, NULL );

- Using "this->" in front of every member access is just syntactic noise.

- If you're using a recent C++ compiler (VS2010 and above), use nullptr instead of NULL. nullptr is a pointer type, NULL is an integral constant. Consider:

void foo(int*);
void foo (int);

void bar() {
  foo (NULL);  // Calls 'foo(int)'
}

I really appreciate your feedback. Especially RAII, which I didn't know about before.

I do have one question though. I am using GCC 4.6.2 to compile the project the code excerpt you quoted in point 3 came from, so it does have (mostly complete) C++11 support and I can use nullptr. With the size of this project, however, it would take considerable effort to convert all of my null pointers to nullptr instead of NULL. What is the advantage of doing so?

[+Asik]

xorangekiller, on 14 September 2012 - 05:11, said:

I do have one question though. I am using GCC 4.6.2 to compile the project the code excerpt you quoted in point 3 came from, so it does have (mostly complete) C++11 support and I can use nullptr. With the size of this project, however, it would take considerable effort to convert all of my null pointers to nullptr instead of NULL. What is the advantage of doing so?

You could just run find-and-replace over the entire project, if you're feeling bold. It should just work, in theory. The benefit of nullptr over NULL is in certain cases where there is ambiguity between integer and pointer type, where using NULL can result in unexpected bugs. nullptr is a proper pointer type unlike NULL, so it behaves as expected.

[Fugi]

I try to comment whenever possible in my code, block comments above classes / methods and small lines for clarification where it may be needed.

I find this method makes it easier when you get stuck, and helps identify logic errors more easily. Not only does it help to maintain the code but it helps me think through a de-bugging process.

If I know somebody else will be reading / maintaining / using my code I make sure I have legible nearly organised comments.

[warwagon]

Steve Gibson Sample Assembly Code, with comments.