OK, so it's a pun on "The 7 Habits of Highly Effective People". But there are a number of programming techniques that I've found virtually instantly identify a defective program. In addition, there are a number of techniques that mark a program as deeply suspect, and likely to break. Finally, there are a number of techniques which represent poor style, some exceptionally poor, which violate principles of modularity and which result in programs that are clumsy to construct, difficult to debug, and impossible to maintain.

Many of these are simple well-known programming issues; for example, the use of constants where a #define is more appropriate. For some reason, I find that there is a tendency of programmers to assign certain integers, such as control IDs, to random hard-coded values. This is a serious problem. But there are a number of techniques that also indicate serious flaws in a program. I will summarize these here, and clicking on one will link to a discussion of it.

Generally, when I get a program from a client who is having problems, I can solve most of the problems by looking for the items I describe here. My track record is that I have been shown code on a page, or on a screen, containing one or more of the techniques I discuss here, and I point to it and say "There's your problem!", usually within the first 30 seconds. It sometimes takes another ten or twenty minutes to explain to the programmer what has gone wrong as a consequence, and demonstrate exactly what can go wrong in the future, but thus far I've never been wrong in my assessment. On one recent consulting trip, I spotted a Sleep() inside a WaitFor..., and said "there's the problem", and then explained "Your program will fail under the following conditions (a), (b), (c) and the behavior it will exhibit on failure will be (d), (e) and (f)" and they said "Yes, that's exactly where and how it is failing!". As it turned out, there was nothing wrong with that part of the program that a complete rewrite wouldn't solve. And the implications on the rest of the program architecture were significant, so they now have several weeks' work to fix the problem, but they now understand why it couldn't possibly have worked, except by a great miracle. I'll talk more about the failure mode details later.

I have a little awk script I run on *.cpp , and it locates these "hotspots". Usually in less than half an hour I can locate more problems than I have been asked to solve just by studying the code near the hotspots. (I consider this a seriously good revenue generator, since instead of the 1-day or 3-day fix expected, I often get a week or more out of it, since the client, like the one described above, says "Yes, we never did figure out why that was failing..." followed by "go ahead and fix that while you're in there"

I've put some Severity notifications in these descriptions. My rating of these is

Potentially fatal: The use of this technique shouldn't work at all. If it appears to, it is an illusion.

The use of this technique shouldn't work at all. If it appears to, it is an illusion. Serious: There is an excellent chance that either the program is defective to start with, or an apparently simple change elsewhere in the program will create a defective program. In some situations, the fact that the program works at all should be considered miraculous.

There is an excellent chance that either the program is defective to start with, or an apparently simple change elsewhere in the program will create a defective program. In some situations, the fact that the program works at all should be considered miraculous. Potentially Harmful: There is an excellent chance that minor changes in the program will result in a malfunctioning program, or necessitate gratuitous changes that a proper program construction would not have required.

There is an excellent chance that minor changes in the program will result in a malfunctioning program, or necessitate gratuitous changes that a proper program construction would not have required. Inefficient : this covers most polling situations. You will be wasting vast amounts of CPU time and accomplishing nothing.

: this covers most polling situations. You will be wasting vast amounts of CPU time and accomplishing nothing. Poor style: While the code works, it represents poor style of programming. It should be cleaned up to something that is manageable.

While the code works, it represents poor style of programming. It should be cleaned up to something that is manageable. Deeply suspect: While the code works, there is almost certainly a better way to accomplish the same task which would be simpler, faster, cleaner, or some other suitably positive adjective. Generally, there are sometimes valid reasons for using the technique specified, but they are fairly rare and easily identified. You probably don't have one of these reasons.

Note that I have not had time to complete all the points in this article, but it has been sitting in my computer for months and I thought it time to finally get it out, finished or not!

Threading

Processes

Clipboard

Timing

Synchronization

Memory Allocation

Dialogs, forms, and controls

I/O

Using synchronous I/O in the main GUI thread for slow devices (serial ports, networks)

Using synchronous sockets

Entering a critical sections around an I/O operations

Using SetCurrentDirectory/_chdir

Program Structure

Severity: potentially fatal

These will result in erroneous programs. The only correct way to exit a thread is to either exit from the top-level thread function (straight worker threads in MFC or pure C), to do a PostQuitMessage from within a UI thread, or do a PostThreadMessage(WM_QUIT,0,0) to a UI thread from outside it.

Why? Because these functions cause the thread to terminate immediately. (Well, from the viewpoint of the thread, "immediately"; the DLL_THREAD_DETACH calls will still be properly made, so it is not as bad as using TerminateThread). Any pending objects on the stack will not have their destructors called. Resources will leak. Worse still, if some of those objects are synchronization objects, such as a CSingleLock or CMultiLock object, the lock will never be released, which will eventually result in the program blocking indefinitely.

So, you claim, you don't do any of this? Sure. Right. That's this week. A year from now, when you are working deep in the thread logic, and discover a failure to synchronize, and add one of these, you are now dead. You just don't know it yet.

Then there's the issue of expectations. When a top-level thread function looks like this

/* static */ UINT CMyClass::threadfunc(LPVOID p) { while(somecondition) { /* thread loop */ DoSomething(); } /* thread loop */ return 0; } // CMyClass::threadfunc

and you decide to add something useful to the completion, such as

/* static */ UINT CMyClass::threadfunc(LPVOID p) { while(somecondition) { /* thread loop */ DoSomething(); } /* thread loop */ wnd->PostMessage(UWM_THREAD_COMPLETED); clean up thread state return 0; } // CMyClass::threadfunc

this is based on the assumption that you will actually exit the thread through the top-level thread function. Any premature exit of the thread handled at some lower level will bypass this code. So the program will occasionally malfunction in bizarre, possibly non-reproducible ways.

Why would you ever want to exit the thread prematurely? Many valid reasons. The file that the thread is supposed to open isn't accessible. The network connection it was managing was disconnected. You had an internal data structure consistency error.

So how do you exit a thread prematurely? Simple: throw an exception. For example, I would write

/* static */ UINT CMyClass::threadfunc(LPVOID p) { while(somecondition) { /* thread loop */ TRY { /* try */ DoSomething(); } /* try */ CATCH(CMyTerminateException, e) { /* catch */ e->Delete(); break; } /* catch */ } /* thread loop */ wnd->PostMessage(UWM_THREAD_COMPLETED); clean up thread state return 0; } // CMyClass::threadfunc

To define your own exception, you can just derive from the general MFC CException class, e.g.,

class CMyException : public CException { public: CMyException() { } virtual ~CMyException() { } };

If you want to pass parameters in the CMyException, just add parameters to the constructor, and appropriate member variables. To throw the exception, just write

throw new CMyException;

In pure C, you should use the __try/__except mechanism and you can only throw integer exceptions, but the technique is the same.

Note that __try/__finally will guarantee cleanup, but only if the thread does not terminate; so while the __finally clause is "guaranteed" to execute, this guarantee applies only if control passes through it. An ExitProcess or ExitThread (or anything that conceals them) will bypass this effect.

Note that in MFC, explicitly terminating a thread is always a potentially fatal operation, because the per-thread handle maps are not cleaned up, including temporary objects. This will leak storage, as well as leaving various kernel objects orphaned. AfxEndThread will actually clean up the MFC state correctly, but has all the other hazards, so don't do it.

Note also that the documentation of _beginthread is misleading; it suggests that calling _endthread "helps to ensure proper recovery of resources allocated for the thread", suggesting that it makes sense for you to do this. Yet it also states that , "_endthread or _endthreadex is called automatically when the thread returns from the routine passed as a parameter", without explaining how it knows to call which function (it is actually because _beginthread will implicitly call _endthread and _beginthreadex will actually call _endthreadex, which you can easily see by reading the source code for the C runtime in

d:\Program Files\Microsoft Visual Studio\vc98\crt\src\thread.c

and

d:\Program Files\Microsoft Visual Studio\vc98\crt\src\threadex.c

(Providing you installed the C runtime source; if you didn't, you should have. Go install it now)

What they really mean is that you should not call ExitThread, which would not clean up these resources. There is no justification for explicitly calling _endthread or _endthreadex yourself.

Severity: potentially fatal

MFC requires certain internal state be set up properly for its correct functioning in a multithreaded environment. This includes creating (and upon completion, destroying) the per-thread handle maps. If you use these methods, you bypass MFC's maintenance of its internal state.

If you call no MFC functions from within the thread, and use no MFC objects in any form, you might get away with using _beginthread, _beginthreadex or CreateThread. But that means that using any MFC function inadvertently opens you to potentially fatal problems. Since there is essentially no reason to use these, you should use only AfxBeginThread.

Severity: serious, potentially fatal

The C runtime comes in two flavors: the single-threaded libraries (normal, debug, and DLL) and the multithreaded libraries (normal, debug, and DLL). It is essential that you use the multithreaded C library (in any of its forms). Unfortunately, the default for a C application is to use the single-threaded library. This library gains some efficiency at the cost of losing all thread safety.

If you call _beginthread or _beginthreadex, the functions will generate a compiler error if you have not gone into Project | Settings | Code Generation and selected the multithreaded library. Therefore, you know that if you get a compilation error, you have to enable the multithreaded library. If, however, you use CreateThread, this is an API function, which is always defined. Therefore you can create threads which use the single-threaded C library from multiple threads.

This technique is perfectly safe if you never call any C library functions from the thread. However, maintaining this restriction is difficult over the lifetime of the program. Since _beginthread or, if you need all the capabilities, _beginthreadex, already provide everything you need to do, there is little if any reason for using CreateThread.

Severity: potentially fatal

This is always a bad idea. Always. It is to my deep embarrassment that in my early writings on threading, in Win32 Programming, I made the mistake of using this. At least I said that I knew it was a Bad Idea, but I was under a fair amount of pressure to finish the book, and was not as careful as I should have been. I have documented the proper ways of terminating a blocked thread in a companion essay.

The problem with calling TerminateThread is that it will instantly terminate the thread, and will even suppress the DLL_THREAD_DETACH calls that many DLLs rely on for correct behavior.

Never use this call. If you think you need it, recode your program so you don't need it.

Severity: inefficient

A common failure mode in multithreaded programming is to have some thread "wait" for another by asking "are we there yet? are we there yet?". All this accomplishes is to waste a significant amount of CPU time, since, unless you have a multiprocessor, the thread that is "waiting" will consume its entire time slice asking for a condition that cannot possibly be set to true because during the time that thread is running, the thread that is being waited for is not running and therefore cannot possibly change the state!

In a multiprocessor, it means the waiting processor simply wastes a significant amount of its time waiting for a thread to finish; that time could be better spent doing something productive, such as running one of the other threads of your application.

It doesn't matter how you poll; some people use a Boolean variable, waiting for it to go TRUE, and some use GetThreadExitCode. Both of these are equivalent, and wrong.

There are two techniques for dealing with thread completion. The simplest method, which I will explain why has serious defects in a different section of this essay, is to simply do a WaitFor... operation on the thread handle. The thread handle remains non-signaled as long as the thread is running (or potentially runnable) and becomes signaled when the thread terminates. Therefore, the thread that is waiting for the completion uses no CPU time while it is waiting. However, since this blocks the waiting thread, and that can be the main GUI thread, this is not generally recommended in the main thread.

The second method is to have the thread PostMessage a completion message (user-defined) to the GUI thread. This can be handled by either a view or the main frame. The thread that receives the message then can enable/disable controls respond to the completion, and so on.

Severity: deeply suspect

A problem that comes up fairly often in the newsgroups deals with the fact that the "GUI is non-responsive", usually to something like a "Cancel" button not being able to cancel a running thread. It is essential that you not block the GUI thread if you expect features like this to work. It is generally a Very Bad Idea to block the GUI thread.

The usual excuse is "but I don't want to have the user start another thread" or something like that. Fine. This is trivial. Just disable the controls that would initiate a new thread. For example,

void CMyView::OnStartComputation() { AfxBeginThread(...); threadrunning = TRUE; }

void CMyView::OnUpdateStartComputation(CCmdUI * pCmdUI) { pCmdUI->Enable(!threadrunning); }

I then use a user-defined message which the thread posts back to the main GUI thread just after the thread loop completes, using

wnd->PostMessage(UWM_THREAD_FINISHED);

In the handler for wnd, add

ON_MESSAGE(UWM_THREAD_FINISHED, OnThreadFinished)

or

ON_REGISTERED_MESSAGE(UWM_THREAD_FINISHED, OnThreadFinished)

LRESULT CMyView::OnThreadFinished(WPARAM, LPARAM) { threadrunning = FALSE; }

For a dialog, read my essay on dialog control management. What I do is

void CMyDialog::OnStartComputation() { AfxBeginThread(...); threadrunning = TRUE; updateControls(); }

void CMyDialog::updateControls() { ... c_StartComputation.EnableWindow(!threadrunning); }

The only difference in the handler for handling the OnThreadFinished event in a dialog is that I call updateControls() explicitly.

For more details on handling user-defined messages, see my essay on message management.

The use of a blocking call on the main GUI thread, or alternatively a long computation, produces unexpected and generally unpleasant effects on the user interface. The failure to put up a wait cursor exacerbates this.

Severity: serious, potentially fatal, inefficient, poor style. (It loses in a lot of ways!)

First rule: PeekMessage is obsolete

Second rule: If you think you need PeekMessage, consult rule 1.

Actually, PeekMessage can be useful, if used with extreme care. For example, the MFC message pump uses it, and that code demands very careful study if you ever think you want to use PeekMessage. It is subtle code. If you cannot be at least as subtle as this code, don't even consider trying to use it.

But in most cases, just pretend you never heard of PeekMessage. Most people who try to use it get it so badly wrong that its very existence has negative effect on program quality. You are always safe if you apply the two rules above. While they are a heuristic for testing the validity of a program, one of my criteria for determining if a program is seriously defective is the mere presence of this API call. In all cases I have seen this used, it has been used incorrectly. So as a simple heuristic I can state that nearly everyone gets it wrong, and therefore, its very presence is a good indicator of a fundamentally flawed program design.

If you think you have to use it, you are almost certainly better off using threads. But I've seen some pretty bad code when threads are used. So if you think you need it in a single-threaded program, your structure is probably wrong, and you should have used multiple threads. And if you think you need it in a multithreaded application, you're almost certainly wrong, since there is nothing meaningful it can do.

For example, here is an adaptation from a piece of code I found on the MFC newsgroup. It is actually a hybrid of several serious errors I've found in multithreaded programs. I decided to discuss all of these in one synthesized example.

// The global variable that controls and reports the thread state BOOL running = FALSE; CRITICAL_SECTION lock; void CMyClass::OnInitThread() { EnterCriticalSection(&lock); running = TRUE; LeaveCriticalSection(&lock); AfxBeginThread(threadfunc, parms); // Do not block the GUI thread while the worker is running while(running) { MSG msg; if(PeekMessage(&msg, 0, 0, NULL, PM_REMOVE)) { TranslateMessage(&msg); DispatchMessage(&msg); } } DoSomething(); }

UINT threadfunc(LPVOID p) { BOOL isrunning; while(TRUE) { EnterCriticalSection(&lock); isrunning = running LeaveCriticalSection(&lock); if(!isrunning) break; ...do thread thing here... if(thread_has_completed_its_work) { EnterCriticalSection(&lock); running = FALSE; LeaveCriticalSection(&lock); } } }

What's Wrong With This Picture?

To be blunt, nearly everything. The AfxBeginThread is salvageable, as is the code represented by the "...do thread thing here...". The test for the thread having completed its work. The function call for doing something when the thread has finished. Nothing else is salvageable. This is a horror of the first order.

First, there is a global variable in use. There is no need for a global variable. There are a lot of ways of dealing with this, but a global variable is an indication there is something wrong with the structure. See, for example, my essays on worker threads, and on callbacks. These show why you would not ever need a global variable.

Then there is the global thread function. This is not necessary. This can be a static class member function (again, see my previously-cited essays on worker threads and callbacks).

The BOOL variable access is carefully synchronized, which is unnecessary, and it is not declared volatile, which is necessary.

But the worst horror of all is the PeekMessage loop with its accompanying comment. This is absolutely, positively incorrect program structure. The failure in thinking here is that the actions which must follow the thread completion must physically follow the thread invocation, in the same function in which the thread invocation appears. This is completely nonsensical. The completion actions are syntactically unrelated to the thread initiation. They are only required to temporally follow the execution of the thread.

The PeekMessage loop is absurd here. It means that while the thread is running, half the CPU cycles go to running a thread that is doing absolutely nothing but consuming CPU cycles in a completely pointless and nonproductive fashion!

The correct solution is to get rid of the PeekMessage loop entirely. There is no justification for its existence.

One of several correct approaches can be characterized by the code below:

class CMyWndClass : public CSomeWindowBaseClass { ... lots of stuff here protected: volatile BOOL running; static UINT threadfunc(LPVOID p); void RunThread(); afx_msg LRESULT OnThreadFinished(WPARAM, LPARAM); }; void CMyWndClass::OnInitiateThread() { running = FALSE; updateControls(); // dialogs only AfxBeginThread(threadfunc, this); }

That's all that is required. Note that there is absolutely nothing else happens in this function! That's because there is nothing else that needs to happen.

When this function returns, the built-in message loop resumes. If there is nothing to do, the message loop blocks on the GetMessage, thus not consuming any CPU cycles because there is nothing else to do. In the best case, if this were the only application requiring service on the CPU, 100% of the CPU cycles would go to accomplishing useful work in the thread, instead of 50% to a useless PeekMessage loop and 50% to the thread. Read: if your task took ten minutes under the previous implementation, it takes five minutes under this one. That performance improvement matters.

Now what does the thread function look like? For details, consult my essay on worker threads, but the code is simply expressed as

/* static */ UINT CMyWndClass::threadfunc(LPVOID p) { ((CMyWndClass *)p)->RunThread(); return 0; // return value never, ever is of interest } void CMyWndClass::threadfunc() { while(running) { /* thread loop */ ...do thread thing here... if(thread_has_completed_its_work) break; } /* thread loop */ PostMessage(UWM_THREAD_FINISHED); // See my essay on Message Management } LRESULT CMyWndClass::OnThreadFinished(WPARAM, LPARAM) { DoSomething(); running = FALSE; updateControls(); // dialogs only return 0; }

This is not only efficient, but note that it does not do any synchronization! Why not? Clearly, the running flag is being accessed by two different threads: the GUI-level thread can set it to FALSE by the user clicking a Stop button or selecting a Stop menu item or any other of a variety of ways. So why don't we need synchronization?

Synchronization is needed only when concurrent access can produce incorrect results. The main GUI thread never reads the value; it only writes it. The worker thread only reads it, but if there is any timing error it really doesn't matter in the slightest. The worst case is that the worker thread might go through the loop one more time if there was a concurrent access that caused the thread to see a TRUE although concurrently there was a FALSE being set. So functionally, this is the same as if the user had hesitated perhaps an extra nanosecond on the click. So no synchronization is required.

What about the argument that "I don't want the user to start the thread(s) again until it(they) finish". Well, the first example code doesn't actually prevent that from happening! So it would be possible to start a new of thread running each time the operation was re-selected. You need some way to disable the initiation of new threads. The simplest way to do this in a view-based app that is using menu items or toolbar buttons is to simply have an UPDATE_COMMAND_UI handler:

void CMyView::OnUpdateStartThreads(CCmdUI * pCmdUI) { pCmdUI->Enable(!running); }

The controls are disabled if the thread is running. For a dialog, or a CFormView, where you have a button on the dialog or form, the updateControls handler I describe in my essay on control management would be

void CMyDialog::updateControls() { ...other stuff here c_StartThread.EnableWindow(!running); }

Note how much simpler the code is. There are no temporal dependencies that are enforced by syntactic sequencing. The system is fully asynchronous, requires a minimum of synchronization (none), consumes no CPU cycles by polling for messages, and is easier to understand.

Severity: potentially fatal

Any variable that is shred between two threads and can be changed concurrently must be marked as volatile. This keeps the compiler from doing certain optimizations, such as code motions, constant propagation and common subexpression elimination, on a variable whose value might change spontaneously.

Note that declare a variable as volatile does not make it thread-safe. But a thread-safe variable, properly handled with synchronization primitives, can still produce incorrect results in the Release version if the variable is not declared volatile.

Example:

while(running) { /* thread loop */ ...bunch of stuff here } /* thread loop */

looks like a perfectly good loop, where running is, for example, a member variable of a class and can be modified by some other thread. For example, you might do

void CMyDialog::OnCancel() { thread->running = FALSE; }

However, the optimizing compiler might detect that within the thread loop, nothing modifies the running variable. If it believes this, it feels perfectly free to transform the program as if it had been coded

if(running) { /* thread loop */ compiler_generated_label: ...bunch of stuff here goto compiler_generated_label; } /* thread loop */

Now, you say, that's ridiculous! That's a completely different program! Not so. From the viewpoint of code optimization the two programs are absolutely identical. This is because the optimizer assumes single-thread semantics. And this assumption is completely wrong.

If, however, you declared the member variable as

volatile BOOL running;

then the compiler recognizes that some mechanism it is not aware of can change the value, and it will not do the transformation.

Severity: potentially fatal

No variable or structure should be accessed from multiple threads without being accessed in a thread-safe fashion. Note that any thread-safe access must also be multiprocessor-safe.

Note that this applies only to variables where concurrent access can produce incorrect results. There are situations in which concurrent access can never result in an incorrect result. One such example is discussed in this essay in the section on the (mis)use of PeekMessage.

However, for situations in which concurrent access can produce an incorrect result, you must provide proper synchronization. This means that even trivial actions like

volatile int a;

a++; // not safe!

cannot possibly work. If you look at the generated code, you will find that it actually takes a couple instructions to increment a, but even if the compiler generated an inc instruction, that takes two memory cycles, which means that in a multiprocessor environment the other processor could simultaneously fetch the initial value. Then both processors would increment the value (say, from 17 to 18) and the value 18 would be stored back, first by one processor, then by the other. So executing a++ twice would cause the value to go from 17 to 18, which doesn't make much sense.

If you believe this cannot happen, you are wrong. I first found this in an operating system in 1968. I found that instead of doing the multithread-safe increment, it did a multithread-unsafe increment. I reported this to my manager, who reported it to IBM. In addition I fixed it. Suddenly, the MTBF of the system went up; in retrospect, examining the "indeterminate" crash memory dumps, we determined that one crash a week happened because there was a thread preemption between the instruction that tested the value and the instruction that set the value.

For simple operations, like increment, decrement, simple addition and subtraction, and a compare-and-exchange, there are Interlocked... operations, so the correct form of the code would be

volatile long a;

InterlockedIncrement(&a);

There are also InterlockedDecrement, InterlockedExchangeAdd, and InterlockedCompareAndExchange, among others.

However, generally you need to provide higher-level synchronization on objects, because very few changes can take place in a single instruction. So you should use CRITICAL_SECTION or a Mutex, or the MFC classes CCriticalSection or CMutex to provide the necessary synchronization.

There is a persistent piece of misinformation which I have said back to me often enough to have realized this has now achieved the status of Programming Legend: by declaring a variable as volatile access to it is implicitly synchronized. Nothing could be further from the truth. The truth is that volatile in no way provides synchronization. What it does do is inform the other threads that they cannot cache the value, which is not at all the same! There is absolutely, positively, no synchronization whatsoever provided by declaring a variable as volatile. You, and you alone, are responsible for providing synchronization. If, however, you also fail to declare the variable as volatile, the cached value may be used by another thread, which would also be incorrect. You must do both.

See my companion essay on synchronization.

Severity: fatal. No "potentially" about it. Programs that are written this way invariably do not even work, from day one.

Basically, there is no reason to do this. Or the reasons are so exotic that you have to be a very advanced user to do this sanely.

Hint: I've been writing threaded code for over a decade and not once, not ever, have I had a need to override the Run() method.

Invariably this is the result of a misunderstanding of the purpose of a UI thread.

The purpose of a UI thread is to have a message pump running. This is important for dispatching events such as socket messages and other asynchronous events from libraries such as SAPI (the speech API). Overriding the Run method will block these messages. The most common failure mode is to put an infinite loop in the Run() method. This can be assumed to make no sense. If you need an infinite loop, use a general worker thread. If you need a UI thread, you do not need an infinite loop in it.

Whatever you are trying to do, overriding Run is going to be the wrong way to accomplish it. Putting an infinite loop in the Run method is always a mistake.

This makes no sense at all. Ever.

It defeats the purpose of having a UI thread. There is absolutely no way this could ever make sense in a UI thread. Use a worker thread. It will block the message pump, which is the only reason you would ever need a UI thread. Therefore it represents a fundamental design flaw.

Whatever was intended by this, the choice of putting it in a UI thread is always a mistake.

Severity: serious, potentially fatal

Never use SendMessage between threads. The result of this can be a deadlock situation. The deadlock can be unpredictable and therefore difficult to debug if it appears to be happening. So the easiest way to avoid this is to never use it.

Use PostMessage to send messages between threads. This also means that a worker thread cannot use most methods on controls, since they all end up being SendMessage calls. Here are just a few of the operations you must not do on a control or window from a thread which does not own it. I don't intend to reproduce the entire list here. Assume that if you are doing anything to a window (or a control, which is just a window) from a thread, except PostMessage, your program is at serious risk.

Note that I've had people say "But I'm not sending any messages to the controls; I use MFC methods!" which is actually the same as saying "I send messages to controls". MFC is just a nice syntactic wrapper on hundreds of SendMessage operations.

Message prefix Example messages MFC equivalents BM_ BM_GETCHECK CButton::GetCheck BM_SETCHECK CButton::SetCheck CB_ CB_ADDSTRING CComboBox::AddString CB_DELETESTRING CComboBox::DeleteString CB_FINDSTRING CComboBox::FindString CB_GETCOUNT CComboBox::GetCount CBEM_ CBEM_DELETEITEM CComboBoxEx::DeleteItem CBEM_GETITEM CComboBoxEx::GetItem CBEM_INSERTITEM CComboBoxEx::InsertItem DM_ DM_GETDEFID CDialog::GetDefID DM_SETDEFID CDialog::SetDefID DTM_ DTM_GETRANGE CDateTimeCtrl::GetRange DTM_GETSYSTEMTIME CDateTimeCtrl::GetSystemTime DTM_SETRANGE CDateTimeCtrl::GetRange EM_ EM_GETLIMITTEXT CEdit::GetLimitText EM_GETLINE CEdit::GetLine EM_GETLINECOUNT CEdit::GetLineCount HDM_ HDM_DELETEITEM CHeaderCtrl::DeleteItem HDM_GETITEM CHeaderCtrl::GetItem HDM_INSERTITEM CHeaderCtrl::InsertItem HDM_SETITEM CHeaderCtrl::SetItem IPM_ IPM_CLEARADDRESS CIPAddressCtrl::ClearAddress IPM_GETADDRESS CIPAddressCtrl::GetAddress IPM_ISBLANK CIPAddressCtrl::IsBlank IPM_SETADDRESS CIPAddressCtrl::SetAddress LB_ LB_ADDSTRING CListBox::AddString LB_DELETESTRING CListBox::DeleteString LB_GETCURSEL CListBox::GetCurSel LB_SETCURSEL CListBox::SetCurSel LVM_ LVM_DELETEITEM CListCtrl::DeleteItem LVM_EDITLABEL CListCtrl::EditLabel LVM_GETITEM CListCtrl::GetItem MCM_ MCM_GETCURSEL CMonthCalCtrl::GetCurSel MCM_GETMONTHRANGE CMonthCalCtrl::GetMonthRange MCM_SETRANGE CMonthCalCtrl::SetRange PBM_ PBM_GETPOS CProgessCtrl::GetPos PBM_SETRANGE32 CProgressCtrl::SetRange32 PBM_STEPIT CProgressCtrl::Stepit PSM_ PSM_ADDPAGE CPropertySheet::AddPage PSM_GETPAGEINDEX CPropertySheet::GetPageIndex PSM_GETACTIVEPAGE CPropertySheet::GetActivePage RB_ RB_GETBANDCOUNT CRebarCtrl::GetBandCount RB_GETBKCOLOR CRebarCtrl::GetBkColor RB_SETBANDINFO CRebarCtrl::SetBandInfo SB_ SB_GETTEXT CStatusBar::GetPaneText CStatusBarCtrl::GetText SB_SETTEXT CStatusBar::SetPaneText CStatusBarCtrl::SetText SBM_ SBM_GETPOS CScrollBar::GetPos SBM_SETPOS CScrollBar::SetPos SBM_SETRANGE CScrollBar::SetRange STM_ STM_GETICON CStatic::GetIcon STM_GETIMAGE CStatic::GetImage STM_SETICON CStatic::SetIcon STM_SETIMAGE CStatic::SetImage TB_ TB_ISBUTTONENABLED CToolbarCtrl::IsButtonEnabled TB_PRESSBUTTON CToolbarCtrl::PressButton TB_SETSTATE CToolBarCtrl::SetState TBM_ TBM_CLEARSEL CSliderCtrl::ClearSel TBM_GETNUMTICS CSliderCtrl::GetNumTics TBM_SETBUDDY CSliderCtrl::SetBuddy TCM_ TCM_ADJUSTRECT CTabCtrl::AdjustRect TCM_GETITEM CTabCtrl::GetItem TCM_SETIMAGELIST CTabCtrl::SetImageList TTM_ TTM_ADDTOOL CToolTipCtrl::AddTool TTM_GETTOOLINFO CToolTipCtrl::GetToolInfo TTM_ NEW TOOLRECT CToolTipCtrl:: Set ToolRect TVM_ TVM_DELETEITEM CTreeCtrl::DeleteItem TVM_GETCOUNT CTreeCtrl::GetCount TVM_GETITEMRECT CTreeCtrl::GetItemRect UDM_ UDM_GETBUDDY CSpinCtrl::GetBuddy UDM_SETRANGE32 CSpinCtrl::SetRange32

Believing a thread starts immediately/Believing a thread does not start immediately

Severity: potentially fatal

Rule 1: any time you assume that a non-suspended thread starts immediately, you are wrong

Rule 2: any time you assume there is a delay in starting a non-suspended thread, you are wrong

Rule 3: there are no exceptions for non-suspended threads. See rules 1 and 2.

An example of the first failure is seen by the number of people who pass a local variable to a thread:

SOMESTRUCT s; s.whatever = somevalue; s.thing = something; AfxBeginThread(threadfunc, &s);

The first failure is that the code will be something like

UINT threadfunc(LPVOID p) SOMESTRUCT * s = (SOMESTRUCT *)p; ...stuff ... = s->whatever; // invalid access

By the time the thread gets around to accessing the s->whatever field, the struct is long gone. The creating function returned, its stack frame is gone, and the space formerly used to hold SOMESTRUCT s is now being used for some other purpose. It gets even worse if the contents of SOMESTRUCT were pointers; they now point to someplace, but where? Who knows? Certainly serious malfunctions will result if they are used to load or store anything.

I've seen a solution posted that looked like this:

UINT threadfunct(LPVOID p) { SOMESTRUCT s = *(SOMESTRUCT *)p;

and the explanation was that by making a copy, the contents would now be saved. Sorry to disappoint the poster, but this is every bit as incorrect as the previous example, and for the same reasons. All you know about a thread, once you've called the ::CreateThread call (by whatever wrapper your environment demands, such as _beginthread, _beginthreadex, or one of the forms of AfxBeginThread), is that at some point in the indeterminate future, the thread will start. And at some other indefinite point in the future, the thread will end.

If you need to pass information across a thread boundary, it must be space that is either statically allocated, or is allocated on the heap, and it must be guaranteed to remain correct for the entire lifetime of the thread. Anything less will not work.

Sometimes you have a single thread that does one thing. In this case, it sometimes can simply use member variables of the class that spawns it. For example, a thread started by a CView-derived or CDialog-derived class may use member variables of the class instance. Note that it is your responsibility to see that this thread has completely terminated before allowing the CView or CDialog to be destroyed. Rather than use individual members, it is often best to have a struct/class which contains all the variables needed for the thread.

Sometimes you need many threads to do certain tasks on behalf of a single view, document, or other class. In this case, it would be impossible to use a single variable or set of variables to hold the state. In this case, you must actually allocate heap memory and pass a pointer to this heap memory to your thread. The memory must be freed by the thread when it has finished using it.

The complementary problem is the illusion that the thread still exists after the return from ::CreateThread or its wrappers. For example, here is a classic error:

CMyThread * t = (CMyThread *)AfxBeginThread(RUNTIME_CLASS(CMyThread), ...); t->m_bAutoDelete = FALSE;

Whoops! If you think this code can work, rethink what you are doing. It cannot work. By the time the assignment is done to t, the thread might have finished, and because m_bAutoDelete was implicitly TRUE to start, the thread object referred to by t is gone, it has been returned to the heap, the space has been reallocated for some other purpose, and you just stored the value "1" somewhere, but you have no idea where! Perhaps you merely corrupted the heap. Perhaps you have damaged some other data structure. You have no idea. None whatsoever. And someday your code is going to crash and burn in a spectacular manner.

This is one of the many reasons that keeping thread object pointers around beyond the immediate creation of the thread is often useless. If you need to set member variables, you need to do

CMyThread * t = (CMyThread *)AfxBeginThread(RUNTIME_CLASS(CMyThread), ..., CREATE_SUSPENDED, ...); t->m_bAutoDelete = FALSE; t->anythingelse = something; t->ResumeThread();

Now you can depend on the fact that the thread has not started running, because it was created suspended. Until you execute ResumeThread, you are safe. Once ResumeThread is called, the thread could be terminated before the call returns. (and if you think this is impossible, you have not thought out what threading means).

One of the common questions which arises under these conditions is "But I can't do x until I know the thread is executing!". If that is so, you need to rethink what you are doing. If you need to do w, start a thread, wait for the thread to be running, and then do x, you have to stop thinking in a procedural fashion and think in an event-driven fashion. The correct approach is to do w, and start the thread. That's it. That's as far as you go. If x needs to be done after the thread starts, why is it not done in the thread? That's the first question to ask. If there is actually a good reason (not just a failure in design) that this must be so, then the correct behavior is to have the thread PostMessage a notification to the main GUI thread, or possibly PostThreadMessage if it was spawned by some other UI thread, and when that message is received, you are now free to do x.

I've seen solutions of the form:

HANDLE h = ::CreateEvent(NULL, TRUE, FALSE, NULL); AfxBeginThread(threadfunc, (LPVOID)h); ::WaitForSingleObject(h, INFINITE);

where the thread contains

UINT threadfunc(LPVOID p) { HANDLE h = (HANDLE)p; ... thread setup goes here... ::SetEvent(h); ... rest of thread goes here... }

which is a bit clunky; it is not a clean solution. Worse still, I've seen the passing of two event handles; after the creator comes out of the ::WaitForSingleObject, it then does something and does a ::SetEvent to tell the thread it can now continue; the thread, after it did the ::SetEvent to resume its creator, did a ::WaitForSingleObject to wait for a resume notification. This is really clumsy, and shows a failure to give up procedural, sequential thought models. Junk code like this, and rewrite it into something simple. Nearly all the time I see code like this, there is actually a much simpler way to write it. The few times a better solution was not feasible, it was because of fundamental design failures elsewhere that could not be easily rewritten. Poor design is poor design, and there is usually not a single point where it goes bad, but lots of places that go bad.

Severity: potentially fatal

Programmers have been erroneously trained to "exit the program if there is an error". Much of this training was simply incorrect, and was often based upon people who learned how to program when programs were encoded in pieces of cardboard and run overnight.

In a modern interactive program it is never appropriate to "exit the program". Exiting the program is an admission of total failure. It is always an error. Therefore, an ExitProcess call, or as it is disguised by the C library, exit, is always inappropriate. If you think you have an "unrecoverable error", you are wrong. All errors are recoverable. The recovery may mean that a whole lot of menu items, controls, etc. are now disabled, but the condition is always recoverable with respect to keeping the program running. To think that such a thing as an "urecoverable" error that necessitates exiting the program could ever exist is a deep flaw in thinking.

I once bought a third party database library, CodeBase. It was without a doubt the worst subroutine library I have had the misfortune to be subjected to. Its attitude was that if an error occurred, there were two errors: those caused by incorrect programming, which would pop up and obscenely cryptic MessageBox, underneath the application because the programmer did not understand that using a NULL handle made the dialog box a child of the desktop, and those errors which were caused by its perception that data was incorrect, in which case it called exit instead of returning an error code that said "data corrupted". They would not even listen to an analysis of what was wrong; they gave lame excuses such as "If the index is bad, we can't proceed". Fine, but that has nothing to do with the program. Any subroutine library that contains an ExitProcess or exit is so fundamentally erroneous that it can only be ignored as a piece of legitimate software. Its inclusion in a project probably dooms the project. After all, do you want to take the call from your customer saying "I was working in the program and it just went away!"? Do you think you can reproduce it?

The arrogance that says a subroutine writer has the right to determine that my program should shut down requires a serious attitude adjustment on the part of that programmer. No subroutine has the right to terminate a program. Only the user has the right to terminate a program. Now, the user may choose to do so because an internal error has rendered the program unusable, but by that time, there should have been detailed analyses of the failure mode presented to the user, the error would have been logged in a way that can be presented to a technical support person, and operations the user might do that would now be invalid should be disabled.

So you say, "If that error occurs, the user cannot proceed!". Fine. This means you end up disabling all the menu items and toolbar items except those which are still viable, such as Exit and About... (the latter is critical if the user calls tech support!)

If an error occurs in a subroutine, return an error code. Or throw an exception. But never, ever call ExitProcess. In addition, in a GUI program, if you call ExitProcess, the process stops. The user gets no chance to decide if buffers should be saved; the mechanism that asks the question has been bypassed; objects which may need to be cleaned up (for example, semaphores, which have an existence independent of the process itself) are left corrupted; Mutexes which are currently locked will generate WAIT_ABANDONED notifications, which guarantee that other processes which are depending on them will also have to enter a nonrecoverable error situation; data which has been buffered but not yet written to the kernel, such as file buffers and record buffers, are lost. Because of these hazards, I have classified this as potentially fatal.

It doesn't matter what you need to do to avoid ExitProcess. Do it. It doesn't matter if the program is a GUI program or an ordinary console app; never call ExitProcess (or exit). I have managed to avoid using this (except for the occasional trivial ten-line test program, and even then I get nervous) for something like 20 years. It isn't hard.

Severity: potentially fatal. If you are lucky, merely serious

Using these on the main GUI thread is almost always an error. The only context in which they ever made sense was in the WM_DESTROY (or the WM_NCDESTROY) handler of a pure C API program. They never made sense in any other context in pure C programming, and they never make sense in an MFC program.

The way to shut a program down is to PostMessage(WM_CLOSE) to the main window. This invokes the standard closedown actions, which typically include such actions as prompting the user to save unsaved buffers. If you get a WM_QUIT message to the message loop, the message loop shuts down immediately and control proceeds to ExitInstance. Nothing else in the program associated with shutdown will take place. This will eventually lead to fatal situations; if not immediately, certainly later (often before the product is released, and if not then, usually on the first round of maintenance).

The only valid place these can make sense is to shut down a worker thread with a message queue, known somewhat erroneously as a "UI" thread, and then only if you have a fair degree of confidence that everything else in the thread is shut down. Usually I will PostMessage a message to the UI thread asking it to shut itself down, and when it has determined it is in a clean state, it does a PostQuitMessage to itself.

The only time an application should shut down is when the user or the system asks it to shut down. This means that the user has used one of the numerous means to ask a shutdown of the app (the close box, Alt+F4, the system menu, the Task Manager), or the user is logging off or the system is shutting down. Otherwise, this has all the charm of using ExitProcess() or exit() and is an error. If the program is seriously broken, you should still keep it running...just don't let it do anything.

Sometimes, shutdown is a multiphase operation. The OnClose handler only initiates the close operation, but does not actually call the superclass OnClose handler which eventually gets to the built-in DefWindowProc which calls DestroyWindow. This is deferred until the necessary conditions are met, such as network connections quiesce, the robotic arm has been retracted to its safe position, and similar long-period delay operations. In this case, the superclass OnClose handler will be called when the necessary conditions are met.

Severity: potentially fatal

Events (those objects created by ::CreateEvent or by using CEvent) can be used for synchronization, but they are inadequate for doing mutual exclusion. The reason is that many threads can pass an Event. For mutual exclusion use a Mutex (created by ::CreateMutex) or CMutex or a CRITICAL_SECTION. If you are using an Event for mutual exclusion, you are using the incorrect mechanism and need to change.

Severity: serious, potentially fatal

A Critical Section of any sort (a section of code which is protected from concurrent execution on the same data item) should be as short as possible. Generally there are a very small number of lines of code between the locking and the unlocking, and furthermore these lines do not usually contain lengthy loops. The longer you spend in a critical section, the higher the probability that another thread will block on the same lock. This decreases total throughput of your program.

But one thing that is always a mistake is to block before a blocking operation. I have actually seen code that locks a buffer in the following fashion (this was in the worker thread).

// in the shared data area: volatile DWORD length; ---------------------------- length = 0; while(reading) { /* read loop */ EnterCriticalSection(&bufferlock); ReadFile(comport, &buffer[length], bufferlength - length, &bytesRead, NULL); length += bytesRead; LeaveCriticalSection(&bufferlock); mainthread->PostMessage(UWM_DATA_READ); } /* read loop */

The main GUI loop was then supposed to read the data by doing

LRESULT CView::OnDataRead(WPARAM, LPARAM) { EnterCriticalSection(&bufferlock); ... use data in buffer... length = 0; // reset the length LeaveCriticalSection(&bufferlock); }

There are two deep and unrecoverable bugs in this code. The programmer had analyzed it as "I can only have one thread at a time accessing the buffer and the length, so I have done proper synchronization". However, the crucial problem was that there was never really a time when the buffer was unlocked. By the time the PostMessage was processed, the buffer was locked again! Consider the execution of the loop: it leaves the critical section, posts the message, loops around, locks the critical section again, and blocks on the ReadFile. So by the time the message is processed in the main GUI thread, the thread is blocked when it tries to enter the critical section. When the worker thread unblocks, it fills in the buffer, releases the lock, posts a message, loops around, and locks the critical section. Boom!

Now the second fatal bug is the fact that there is no test for buffer overrun. That is, if you look at this, it just keeps reading data from the serial port. Because the main thread can never get a chance to reset the position to zero, it merely keeps increasing. When the position gets to be greater than bufferlength, the computation bufferlength - position gives a negative number, but since the input is a DWORD, this turns out to be a massively large positive number. Since the buffer is not as long as the large negative number suggests, the inevitable result is that the I/O Manager will reject the ReadFile by throwing an access fault exception.

The trick here is to never, ever lock the buffer during the I/O operation. The best method would be to have the thread allocate an input buffer on the stack, and upon completion, allocate a block of storage to hold the data and PostMessage a pointer to the heap block to the main GUI thread which handles it. This is similar to the technique that I describe in my essay on worker threads, except I wouldn't use CString * for the data because the data could contain embedded NUL bytes, which would be incompatible with CString. I might PostMessage a simple BYTE * pointer in WPARAM and the length in LPARAM, or I might use a CArray<BYTE, BYTE> *, or, if I were a user of STL, some STL array structure. At no time would any locking be required.

Alternatively, it is worth observing that if I put a lock simply around the setting and examination of the buffer length, there would only be a very small window of locking, and furthermore the locking would essentially be guaranteed to complete in a nominally brief period of time (excluding effects of thread priorities). However, I would be disinclined to use this method when a simpler method already exists. Note that the performance issue doesn't arise because the entire allocation and message handling task completes in less than a single character time at any serial port speed, and would therefore be undetectable. Again, refer to my essay on "Optimization, Your Worst Enemy". Why introduce complexity when it is not needed?

Severity: poor style/potentially fatal

There are some very rare occasions in which it makes sense to change the working directory. Every one of them is at the explicit request of the user. It is never appropriate to simply change it. Possibly the worst example of the misuse of SetCurrentDirectory (or its C-library equivalent, _chdir) is in programs that try to do recursive tree walks during wildcard searches. The problem with this call is that it changes the working directory for the entire process, meaning that every thread sees the effect of this. Since many threads open or create files based on the current directory setting, a thread that creates a file while another thread is changing the directory will end up creating it in some random place.

The changing of a directory should only be a consequence of the user requesting a change to a specific directory. Changing it at any other time, for any other reason, that does not involve the user explicitly specifying the directory (often as part as executing one of the file dialogs or a ShBrowseForFolder dialog), can be thought of as a serious error. The programmer does not have the right to spontaneously change the current directory. To do so is at the very least exceedingly poor style, and at worst it can have potentially fatal consequences if you have multiple threads.

Severity: inefficient

A common failure mode in multithreaded programming is to have some thread "wait" for another by asking "are we there yet? are we there yet?". All this accomplishes is to waste a significant amount of CPU time, since, unless you have a multiprocessor, the thread that is "waiting" will consume its entire time slice asking for a condition that cannot possibly be set to true because during the time that thread is running, the thread that is being waited for is not running and therefore cannot possibly change the state! Putting this thread in a separate process is no different from polling for thread completion in the same process. GetExitCodeProcess is as pointless as GetExitCodeThread. If you want to see the process exit code, you do this after you have determined the process has finished. Since most processes don't return a meaningful code, even that is usually pointless.

In a multiprocessor, it means the waiting processor simply wastes a significant amount of its time waiting for a thread to finish; that time could be better spent doing something productive, such as running one of the other threads of your application, or some other application, or a thread of the application you are waiting for.

There are three techniques for dealing with process completion. The simplest method, which I will explain why has serious defects in a different section of this essay, is to simply do a WaitFor... operation on the process handle. The process handle remains non-signaled as long as the process is running (or potentially runnable) and becomes signaled when the process terminates. Therefore, the thread that is waiting for the completion uses no CPU time while it is waiting. However, since this blocks the waiting thread, and that can be the main GUI thread, this is not generally recommended in the main thread.

The second method is useful when you have a console-style app for which you have redirected stdout, which you are receiving via an unnamed pipe in your application. When the pipe is broken, it typically means that the application has finished, since very few applications would spontaneously close stdout. It is usually closed implicitly when the process finishes.

The third method is to create a thread that waits for the process to complete, and posts a user-defined message to the appropriate window (view or mainframe) to indicate that the process has finished. This is discussed in the next section and in an accompanying essay.

Severity: inefficient

I find this a pointless operation. The whole idea of a critical section is that you should block if you can't enter it. But here is a call that simply returns FALSE if you can't enter the critical section. What can you do? Well, retry the operation. This is very much like polling. If you need a timeout, use a Mutex and a WaitForSingleObject with a timeout. So I find that this does not seem to solve any problem that matters, but the few times I've seen it used it results in a merely inefficient program, and in one case an erroneous program.

And, I should point out, that the following are not equivalent:

::WaitForSingleObject(mutex, timeout);

while(!TryEnterCriticalSection(&lock)) Sleep(deltaT);

In the first form, the wait blocks the thread until either the mutex handle becomes signaled or the timeout occurs, whichever occurs first. In the second form, if the TryEnterCriticalSection fails, the thread always blocks for the deltaT time. If the lock becomes signaled 0.1ms after the TryEnterCriticalSection, the loop still waits the full deltaT before testing again. Think about it This is like calling up tech support, finding there is someone ahead of you in the queue, and waiting five minutes before calling back. There is an excellent chance you will never, ever get support.

Severity: potentially fatal

This is almost always an error. For one thing, if you are capturing output from the process, you won't be able to display it, because even if the I/O is done in a separate thread, the updating of the main GUI controls is handled by the GUI threads. So the net result is that the program appears to not work at all. Never block the main GUI thread if you can help it. Certainly never block it waiting for process completion.

Note that this is more serious than just blocking a thread and getting a non-responsive GUI, which is merely annoying to the user. Your whole program can simply hang. So it goes beyond poor interface strategy and falls into the potentially fatal class.

As with asynchronous thread notification, you simply disable all controls, menu items, etc. that are inappropriate while the process is running. You've seen this happen when you run a Build under the VC++ IDE. If you drop down the Build menu, there are lots of menu items enabled, but Stop Build is disabled. If you select a build option and the compiler is running, all the other options are disabled but Stop Build is enabled. This is simple OnUpdateCommandUI handling.

What I do is have a Boolean that enables/disables the controls. It is set to TRUE when a process is running and FALSE when a process completes.

class waitInfo { public: waitInfo(HANDLE p, CWnd * wnd) {process = p; recipient = wnd; } HANDLE process; CWnd * recipient; }; ...AfxBeginThread(waiter, new waitInfo(procinfo.hProcess, this)); processRunning = TRUE; /* static */ UINT CMyView::waiter(LPVOID p) { waitInfo * info = (waitInfo *)p; ::WaitForSingleObject(info->process, INFINITE); recipeint->PostMessage(UWM_PROCESS_FINISHED, (WPARAM)info->process); delete info; return 0; } LRESULT CMyView::OnProcessFinished(WPARAM, LPARAM) { processRunning = FALSE; return 0; } void CMyView::OnRunProcess(CCmdUI * pCmdUI) { pCmdUI->Enable(!processRunning); }

Severity: fatal. No "potentially" about it.

These are functions that are such an outrageous kludge that I'm not going to dignify them with an explanation of how they work internally. They don't work, never did. The only reason they gave the appearance of working was the incredibly poor quality of the PDP-11 C compiler, which ran in 64K of memory and generated lousy code. The technique couldn't possibly work in any modern optimizing compiler, and didn't. Been there, done that.

The essence of setjmp was that it marked a place to which control would transfer when a longjmp was executed. It would return with 0 when called directly and nonzero (the longjmp value) if it was an abnormal return.

Today, this is handled with exceptions. setjmp is replaced by TRY/CATCH and longjmp is replaced by throw. Key here is that in C++, these are first-class concepts in the language and not subject to the horrendous bugs that setjmp/longjmp generated in most compilers (the compiler was working correctly; the longjmp logic would produce erroneous results). But the major failure mode of longjmp is that it bypasses all destructors on the stack, with inevitably fatal consequences.

Bottom line: if you have setjmp/longjmp in a C++ program, you have a fatally flawed program. Remove them and replace them with C++ exception handling.

Note: If you believe there is any justification for setjmp/longjmp in a C++ program, you are wrong. Don't try to justify the situation, fix it. (Seriously, I've had people argue this point with me. It takes less time to fix the problem than to try to justify why it makes sense).

Severity: potentially fatal (except if the GUI program is written in C++, in which case it is always a fatal error)

Yes, I've done it. In Win16. With optimizations turned off, because the brain-dead third-party library we were using demanded that we support it. I would never support it willingly. There is no excuse for using it in Win32. The problem in a GUI program is that you can end up with state that is not cleaned up, operations that do not complete, and a plethora of other problems. And I cover these in the next section.

Severity: potentially fatal (except if the program is written in C++, in which case it is always a fatal error)

If you are programming in C, use Structured Exception Handling. If you want portability, you cannot tolerate setjmp/longjmp in any case because it is only barely functional. To make it work correctly, you must declare every local variable as volatile, and that's just the start. You must not use the register qualification on any local variable or parameter. There are limits how setjmp can be called. There are nonportable limitations on the contexts in which longjmp can be called. Managing the longjmp buffer is a real royal pain. They don't nest without a lot of effort. Given we now have linguistic mechanisms that get rid of the need for this horror, it is best to avoid it.

Essentially, if I see this in a program, I know the program is working only by amazing good fortune.

Severity: fatal. This is so incredibly bogus that it is impossible to classify it as anything else.

No, it isn't fatal to your program. Programs that violate this rule will probably continue to work just fine. Just don't ever be caught in a dark alley with one of your users!

You, the programmer, do not have the right to modify the clipboard contents. Only the user has the right to modify those contents. Thus, the only time you are permitted to modify the contents of the clipboard is when the user has requested you to by doing a copy-related or cut-related action. You may not modify it at any other time. I've seen people use it for interprocess communication. This is absolutely beyond any shadow of a doubt the WRONG thing to do. There is no way to emphasize this strongly enough. Only if the user has an explicit operation that says "Put this in the clipboard" are you permitted to change the clipboard contents. It could be a menu item, a popup menu item, Ctrl+C, Ctrl+X, a pushbutton that says "copy", or anything else. Then, and only then, are you permitted to change the clipboard contents.

Imagine the following: your program is running. It fires off another program to run. It puts something in the clipboard to send to that program. However, I left out something in this sequence. The real sequence is

Your program starts

While your program is running, the user opens some other document, probably from another application

The user does a Cut operation

The user closes the document, saving the contents which have had something removed

Your program drops trash into the clipboard

The user brings up another app and does a Paste operation, expecting to get the valuable information which has been cut to the clipboard

The user puts out a contract on you

Severity: potentially fatal

There are valid reasons for using Sleep. However, the most common usages I find of them are completely wrong-headed and in most cases result in fatal flaws in the programs.

First, you have to understand what the value of n means: it means, "Stop this thread for at least n milliseconds". The thread will resume sometime after that. Perhaps a long time after that. So if you say "Sleep(5)" the likelihood that your thread will be running 5ms later is nearly zero. This is because the basic clock is 10 or 15 ms (or in MS-DOS, 55 ms, but I consider those systems dead), so it cannot possibly run sooner than one clock tick. But if a higher priority thread is running, it will not be preempted, so it might be several timeslices before the thread which is now runnable can actually run. Even if you think you have control of the priorities, you don't, because the Balance Set Manager thread may be running, and it can boost any thread to priority 15 and give it a double timeslice, and in NT, there are file system and other kernel threads, and Deferred Procedure Calls (DPCs) that will also preempt. Read my essay "Time is the simplest thing" for more details.

Mostly, when I spot a Sleep() call in a program, I ask "Why is this here?" and the answer is "Because the threads won't work unless it is". What this means is the multithreading architecture is irrevocably flawed and needs to be rethought and perhaps rewritten. Throwing a Sleep() call may give the illusion that you have solved the problem, but in fact there is a fundamental problem, and the Sleep() call disguises it. However, on a faster machine, or a different version of the operating system, or a different mix of applications, or a different loading factor on your app, or most especially, on a multiprocessor, you will discover that the fundamental architecture is flawed. At that point, you don't have a choice.

True story: some time ago I was consulting with a client, doing a code walkthrough of their multithreaded application. I spotted a Sleep() and knew immediately that this was where the problem was. I didn't know what the problem was, but I knew I had found it. After some explanation by their programmer (which included "the threading doesn't work right if it isn't there") I understood the code, and said "This will fail under the following conditions" and proceeded to outline the conditions, and said "and it will fail in the following ways" and listed a set of ways the program would fail. They were impressed. I had described exactly the conditions under which it failed and the manner of the failures, without ever having seen the program run. It had taken me perhaps 15 seconds to spot the Sleep() call (they had said "Here's the function that is failing" so I didn't have to look for it). How did I do this so quickly? Because it was by no means the first such instance I had seen! And in every previous time I had been absolutely correct.

Another case I see all too often is after a ::CreateProcess call there is a Sleep() call, "to give the process time to come up". No matter what value is chosen here, it is wrong. There is no correct value for this Sleep() call! Either it is too short, or too long, but it will never be "just right". There is a simpler method if you are waiting for a GUI app to become active, with its window fully displayed. Use the WaitForInputIdle API call. You can put a timeout on it, but this timeout is usually fairly large and is intended to indicate that you are in really deep yogurt, somewhere down in the boysenberries. If the timeout is selected properly, it will be fairly long, and it means that the process has failed to come up at all.

There are occasional cases in which a Sleep() makes sense. But they are so rare, and so special-case, that I can't give a general rule by which you can determine that it is a valid usage. But keep in mind that most uses are not valid or sensible. And also it means that any constant you choose for the timing value is probably wrong, also. In my multithreading lab, I demonstrate a case of using Sleep(5000) to make the program work. I then force the students to consider all the ways this can fail, and why any value, 5000, 10000, 1000, etc. will always be wrong. Then I force them to rethink how the synchronization between the threads is done. When we are finished, we have a correct, Sleep-free program.

There are times when Sleep(0) is handy to produce interesting animation effects in multithreaded programs; Sleep(0) forces the current thread to yield and allows another thread to run. This is in general inefficient, because it induces a lot of gratuitous context swaps and scheduling operations, but for some animation effects to illustrate the effects of multithreading it really makes a nice visual effect. But you have to be willing to give up a fair amount of CPU resource to use this, and it is very special case.

Severity: potentially fatal, or absolutely essential

SleepEx() has all of the charm of Sleep(). It differs only slightly in that it takes two parameters; one of which is the sleep interval and one of which is a Boolean indicating if pending Asynchronous Procedure Calls (APCs) should be taken. The combination SleepEx(0, TRUE) has the characteristic that it does not really suspend the thread, but allows any pending APCs to be taken. If there are any APCs, they will be processed before the SleepEx continues. Thus, if you are using asynchronous I/O, this is actually not only a useful call, but in some ways an essential call (You could also use WaitFor...Ex() calls). So you can feel free to use it in the context of pending asynchronous callback I/O. If you are not using one of the WaitFor...Ex() calls you must use SleepEx(n, TRUE).

Severity: inefficient to potentially fatal

This is one of the most common errors I've seen. The idea is to somehow ask the system about how much time has elapsed and then take some action based on a change in time. Most of the code I've seen to do this is usually bizarre or convoluted. And mostly seems to be based on some illusions about what time is. As a prerequisite to doing anything with time, you should read my essay on time.

First, you can't poll for time. This just wastes CPU time with no guarantees that anything meaningful will happen.

As pointed out in my essay, the resolution of a time value has nothing to do with the precision that is used to represent it. For example, a time value that can be stored in milliseconds. I've seen code that goes like this

SYSTEMTIME t0; GetSystemTime(&t0); while(TRUE) { /* pause loop */ SYSTEMTIME now; GetSystemTime(&now); if(abs(t0.wMilliseconds - now.wMilliseconds) > 1) { /* 1 ms passed */ break; } /* 1 ms passed */ } /* pause loop */

Actually, this code was written because the programmer said "But you said that the resolution of the Sleep() function was only 10ms, so this gets around that!". Actually, the system time increments by some number of milliseconds on each clock tick, and consequently if you write a program that simply reads the time in a tight loop, the minimum difference you will see is 10ms or 15ms. So this code is identical to Sleep(1) without the simplicity, and will resume sometime between 1ms and 10ms after the call. So the above code is fatally flawed. Not only is it inefficient, it doesn't do what it claims to do.

An even worse case was one where the programmer wanted to activate something at 8 a.m.. The code was similar to this:

void DoAtEight() { while(TRUE) { /* Wait loop */ CTime t = CTime::GetCurrentTime(); CString s = t.Format(_T("%H:%M:%S")); if(s == _T("08:00:00")) { /* 8am */ ...do processing...; } /* 8am */ } /* Wait loop */ }

Now this code cannot possibly work reliably. First, the programmer observed that the machine ran somewhat sluggishly and Task Manager showed that the application was consuming 100% of the CPU time. This should not be surprising; the code sits there and keeps asking "are we there yet?" "are we there yet?" and will only be suspended when it has exhausted its timeslice. But what is worse, this code cannot work. Consider: a timeslice is about 200ms (the details vary based on the fundamental time tick). So suppose that at 07:59:59.99, the last time read, the thread executing this code is suspended. There are ten other threads waiting their turn. The next time the waiting thread is scheduled, it reads the time, and finds that the time is now 08:00:01.13. So at no time will the string representing the time ever be 08:00:00. Now on some days this will work, but on other days it will fail. In addition, in the unlikely chance that the time read was actually 08:00:00, if the processing takes less than one second, there is an excellent chance that the next time through the loop the time might again be 08:00:00, causing the processing code to be executed two or more times, instead of the once that was desired. All in all, possibly the worst possible way to accomplish the task.

My proposed solution was to do something like this:

void DoAtEight() { BOOL processed = FALSE; while(TRUE) { /* wait loop */ CTime t = CTime::GetCurrentTime(); if(t.GetHour() >= 8 && !processed) { /* process it */ processed = TRUE; ...do processing...; } /* process it */ else if(t.GetHour() < 8) processed = FALSE; Sleep(60000); // wait for a minute, or a bit more } /* wait loop */ }

This is still not optimal; it wakes up once a minute, performs its test, and goes back to sleep. It would be even better to use a waitable timer.

void DoAtEight() { HANDLE timer = ::CreateWaitableTimer(NULL, FALSE, NULL); BOOL processed = FALSE; while(TRUE) { /* wait loop */ SYSTEMTIME t0; ::GetSystemTime(&t0); if(t0.wHour > 8) { /* adjust to tomorrow */ ULARGE_INTEGER t; ::SystemTimeToFileTime(&t0, (FILETIME *)&t); t += 24ui64 * // hours in a day 60ui64 * // minutes in an hour 60ui64 * // seconds in a minute 1000ui64 * // milliseconds in a second 1000ui64 * // microseconds in a millisecond 10ui64; // 100 ns units in a microsecond ::FileTimeToSystemTime((FILETIME *)&t, &t0); } /* adjust to tomorrow */ t0.wHour = 8; t0.wMinute = 0; t0.wSecond = 0; t0.wMilliseconds = 0; LARGE_INTEGER alarm; ::SystemTimeToFileTime(&t0, (FILETIME *)&alarm); ::SetWaitableTimer(timer, &alarm, 0, NULL, NULL, FALSE); ::WaitForSingleObject(timer, INFINITE); ...do processing... } /* wait loop */ }

Severity: poor style, potentially harmful

There is no reason I can imagine to use malloc or free in a C++ program. If you need them, there are useful alternatives. For example, if you need a dynamically-allocated buffers of n bytes, the simplest way is not to write

BYTE * buffer = malloc(n);

but to write

BYTE * buffer = new BYTE[n];

They perform the same action, so why use an obsolete allocation mechanism.

Why does this matter? Consider the following: I need an array of objects, so I allocate them as follows:

typedef struct { int x; int y; } Coordinate;

Coordinate * coordinates = (Coordinate *)malloc(n * sizeof(Coordinate));

Looks good, right? Works fine, right? Sure. This week. But now I change the structure to

typedef struct { int x; int y; CString description; } Coordinate;

Now what happens? The malloc operation does not call the CString constructor. You have garbage. Your program crashes.

The correct solution was to have always written the code as

Coordinate * coordinates = new Coordinate[n];

This works, and it will always work correctly, even if you later add members to the structure that require constructors. Writing code that breaks when apparently irrelevant changes are made is always a Bad Idea. And since there is no possible justification for using malloc, why bother writing incorrect code?

Yes, I've heard the argument, "malloc is faster". I have some advice for anyone who advocates this: Get A Life. You are clueless. An allocation takes thousands of instructions. The trivial overhead introduced by using new is unmeasurable. This is a classic example of the "let's optimize at the instruction level" when the real costs are at the architecture level. If you are allocating frequently enough that you think this trivial overhead can matter, you are very confused; why are you allocating at all? The real issue is to make programs efficient, and you don't accomplish that by worrying about unmeasurable overheads when the real costs (such as the actual cost of the allocator) swamp it by two to four orders of magnitude.

An even worse situation I've seen is the mixing of malloc with delete, or even new with free. There is no possible justification for such insanity. If it exists, you are wrong. Fatally wrong (no "potentially" here!). Fix the code. Pretend malloc and free are obsolete and should never, ever be used in a C++ program.

Severity: inefficient, deeply suspect (except those cases where it is mandated)

These APIs are essentially obsolete, with a very few exceptions. Only when you have one of these exceptions should you ever consider these as viable entities. Otherwise, use new/delete. They are relatively inefficient in terms of both time and space.

The contexts that I am aware of are the clipboard, where you must provide a global memory handle This must be allocated by GlobalAlloc. You use GlobalLock to make its address available, GlobalUnlock when you are done with it, and you never call GlobalFree on a clipboard handle, under any circumstances.

There are certain uses with Blobs in OLE controls that use GlobalAlloc.

The DEVNAMES structure and certain fields of the PRINTDLG (contained in CPrintDialog) use global memory handles for backward compatibility with 16-bit Windows.

There may be other uses, but the documentation will always tell you if a global handle is needed. Otherwise, avoid them entirely.

Severity: poor style

There is no reason to use LocalAlloc. This is a throwback for compatibility with old Win16 programs. Ignore its existence. The only reason to use LocalFree is for the buffer that is returned from FormatMessage. There may be other uses that are mandated, since I have not read or memorized all 5,000 or so API calls. But unless the use is mandated by an API call, there is absolutely no reason to consider this as useful.

Severity: potentially fatal

When you do a delete on an array of objects, it is crucial that you include the [] in the syntax, that is, if data is an array of objects,

delete data; // wrong! delete [] data; // right

The problem is that the first form only deletes the space occupied by the array. The second for deletes the space occupied by the array, but first calls the destructors on every object in the array.

For example, consider the class

class MyData { public: int x; int y; };

If I write

MyData * data = nwe MyData[n];

then n objects are allocated. If I do

delete data;

the array is deleted. Everything appears to work. But consider, if I do

class MyData { public: int x; int y; CString s; };

The the call

delete data;

will delete the space, but the CString destructor will not be called on each element. The effect will be a storage leak. If, however, I write

delete [] data;

then the destructor is called on each element of the array, and proper object destruction is done for each contained object. Note that the effect of failing to write the delete correctly is that the addition of an object requiring a destructor will mean the program stops working properly in some way (usually a memory leak, but it could be more serious), and you won't know what went wrong.

Severity: deeply suspect to potentially fatal

By "static" variables here I mean a declaration of the form

static type name;

and all variants, when they appear inside a function. An example is

int DoSomething() { static int count = 0; ... count++; }

This may give the illusion of being modular, but in fact it is a very poor programming technique. I find that this is a technique which is a throwback to the more primitive forms of C programming. It is inappropriate in a Windows programming environment. The reasons deal with functions that need to be reentrant across class instances and functions which could appear in a multithreading context.

The worst and most typical form is

LPCTSTR ConvertToString(SomeType * t) { static TCHAR result[SOME_SIZE_HERE]; wsprintf(_T("%d:%02d/%d"), t->seconds / 60, t->seconds % 60, t->divisor); return result; }

A piece of code like this in any program is an invitation to disaster. The first thing I do when I find it is rip it out and replace it with something sensible. There is no point in wasting time trying to debug a program that is this deeply flawed. When I'm working with MFC, I tend to return CString data types, since the allocation is automatically handled.

When would the above code fail? Consider the case

CString s; s.Format(_T("%s and not greater than %s"), CovertToString(t1), ConvertToString(t2));

Both ConvertToString operations use the same buffer and return it, so by the time the format string is processed, the buffer computed by the conversion of t2 is clobbered by the result of the conversion to t1 (remember that parameters are always evaluated right-to-left).

Consider the case of multithreading, where two threads execute a call to ConvertToString. The value seen is unpredictable, and on a multiprocessor the string could potentially change while being read!

The use of const static variables is perfectly acceptable, for example,

double ConvertToSomething(double f) { static const factors[] = {1.0, 1.5, 1.723, 1.891, 1.9843}; ... do some conversion here, using factors[i] based on ... some criterion appropriate return result; }

The code of the form

CString cvMonth(int n) { static const LPCTSTR names[] = {_T("Jan"), _T("Feb"), _T("Mar"), ...etc }; return names[n]; }

however, is probably a Very Bad Idea since it binds a particular language into the source code, which is a losing idea when internationalization is required. You could do

CString cvMonth(int n) { static const UINT names[] = { IDS_JAN, IDS_FEB, IDS_MAR, ...etc. }; CString result; result.LoadString(names[i]); return result; }

But even this is a Bad Idea if you have to deal with any culture that has more than 12 months in its year, or whose month index would not correspond to the Western European/American civil conventions that January is the first (zeroth) month. Use the National Language Support APIs, but that's a different essay to be written at some future time.

There is one exception to this: cached values. A cache keeps a precomputed value, but--and this is critical--it has a way of detecting if the cache is invalid. You can frequently get nice performance speedups if you use cached values, but here you have to weigh the cost of cache management with the cost of not caching. This management includes both the execution costs of determining if the cache is valid and the architectural costs imposed if explicit cache invalidation is required (and the concomitant costs of failure to invalidate and using incorrect data). Here you have to use good judgment, and put lots of explicit comments explaining exactly why the use of a non-const static value is a valid thing to do. And even then, I'd be more inclined to keep the cache in a member variable of a class that was involved in the computation, rather than in a static variable. The only reason to use static variables is if the cache maintains validity across multiple objects.

For some reason, people use new when there is absolutely no reason to do so. The most common failure of thinking is when it comes to passing pointers to objects. For example, if I have an API that requires a parameter SOMEDATATYPE *, the solution is obvious:

SOMEDATATYPE data; SomeAPI(&data);

This has not stopped programmers from thinking that the type of the argument has to match the type of a variable, so they will do

SOMEDATATYPE * data; data = new SOMEDATATYPE; SomeAPI(data); ... use it delete data;

It is hard to imagine why the second form could ever be imagined to make sense. It introduces a gratuitous concept, memory allocation, to a place where memory allocation is not needed, and requires a corresponding delete. This not only adds intellectual baggage, it obscures the purpose of the code, and introduces two possibilities for error. The first is that the new failed (note the lack of any check for its success!) and the second is that in the section called "use it" that the code gets very long, and someone puts, in a later modification, a return FALSE or some similar construct into the code which bypasses the delete, resulting in a storage leak.

Another example is to create a member variable for some type of window, e.g., a dynamically-created CButton:

class whatever : public CFormView { ... protected: CButton * mybutton; };

then in the code do

mybutton = new CButton; mybutton->Create(...stuff here...);

This has the same problem as above. This introduces a gratuitous need for a concept that has no relevance, allocation; it requires that you remember to put into the constructor an assignment

mybutton = NULL;

and in the destructor

delete mybutton;

all of which is pointless intellectual overhead. Instead, do

protected: CButton mybutton;

and then you can do

mybutton.Create(...stuff here...);

Note that concepts such as initialization of the pointer, destruction of the object, and the use of new are replaced by a trivial concept: declaring a variable. There is virtually no reason to allocate a CWnd-derived class dynamically when its extent is the extent of the parent class.

Just about the only place where you do this dynamically is to allocate a CDialog *-derived object for a modeless dialog.

Another example is "but what if I need an array of integers?" What if you do? CArray or std::vector exist, and work quite well. For example, consider the case where you need an array of integers which represent the selected items in a multi-select listbox:

CArray<int, int>selections; int n = c_MyListBox.GetSelCount(); selections.SetSize(n); c_MyListBox.GetSelItems(n, selections.GetData());

works nicely, and has the advantage that you don't need to remember to call delete.

There are lots of valid reasons to use new. Foremost among them is passing information across thread boundaries. Information to be passed is put into a heap-allocated object, and a pointer to that object is passed from one thread to another. When the receiving thread is finished with the object, it calls delete.

Severity: deeply suspect, poor style (unless there really is no alternative!)

There are lots of valid reasons for creating controls dynamically. One correspondent pointed out that he had to create controls based on an XML description. I've done them from database schemata. But 99% of the time I see someone creating a control dynamically, they're doing it for the wrong reasons. And sometimes when they do it, they do it in the wrong way.

It is rare to need to create a control dynamically (in spite of the fact there are lots of valid reasons, they only rarely apply). However, it is also unnecessary in general to do a new to allocate an object to reference the control. I commonly see people doing things like

BOOL CMyDialog::OnInitDialog() { CDialog::OnInitDialog(); if(sometestorother) { /* we could stop */ CButton stop = new CButton; CRect r(175, 54, 205, 65); stop->Create("Stop", BS_PUSHBUTTON, r, this, IDC_STOP); } /* we could stop */

Just about every line in the above code is about as wrong as you can get. The problems are

You don't need to do a new

You would have to do a delete but this code stores the pointer in a local variable so the window could not be deleted

but this code stores the pointer in a local variable so the window could not be deleted The magic constants shown are complete nonsense

It only works for 8-bit English applications

It actually assumes the the Create operation works!

Key here is that none of these lines are required, or are even correct. I would simply create the control in the Dialog editor, size it and place it where I wanted it, and then do

BOOL CMyDialog::OnInitDialog() { CDialog::OnInitDialog(); c_Stop.ShowWindow(sometestorother ? SW_SHOW : SW_HIDE);

Note how much simpler and cleaner this is. I don't have to compute a position, I don't have to allocate anything, I don't have to delete anything, and I don't have to do a Create.

If I had to create a control at runtime, there is no reason to do a new if there is not some dynamic quantity being created. Just declare yourself, outside the scope of the magic AFX comments, an appropriate variable

CMyCustomControl ctl;

Don't do a new unless there is a compelling reason to do so (there can be, but most of the instances I've seen where new is used to create a control variable are just a waste of conceptual energy).

When you need to create the control, just make sure your custom control has a Create method, and write

CString caption; caption.LoadString(IDS_STOP); VERIFY(ctl.Create(caption,and,parms,you,want,here));

To create a control based on a custom window class, you can select the "custom control" (the little human head icon) from the toolbox. Use it to place the control. Specify the class name of the control class in the Properties box. Select the desired styles. The problem with the styles is that you can't just use symbolic names like WS_VISIBLE, which is a real pain; you have to go into winuser.h and figure out the correct hex constants to use. Sigh. Make sure you have registered the window class before you try to create the window. You may do this in the InitInstance handler for a dialog-based app, or in the constructor for a CDialog- or CFormView-derived class. Or see my essay on self-registering classes.

I use VERIFY to make sure that if there were an error, I will see it at the time the creation is done, instead of having some other part of my program fail much later for reasons I will have to work hard to understand. In some cases I might store the result and take other action, but in most cases, creation failures represent coding errors which, once fixed, no longer exist. This catches the failure early.

Hints: making dynamic creation easy

Suppose I wanted to create a pushbutton. I would know that, for example, its left edge, right edge, top edge, and/or bottom edge (choose at least one horizontal and one vertical) would have to line up in some way with some other control. Or be a certain percentage distance from a left, right, top, and/or bottom of the window.

Rather than compute the button size, which must be in pixels, based on some criterion, or trying to mess with Dialog Box Units, which are a waste of intellectual effort, I would use one of several tricks.

When I had to do this in a product I delivered, I created a form which had several hidden controls: a combo box, a radio button, a check box, a static label, and an edit control. I created these as disabled, invisible controls. I did not have space on the form to put them (the entire body of the form was a giant ListBox that contained these controls inside it), so I simply made the ListBox a little smaller vertically and resized it in the OnInitialUpdate handler of the form. Thus it overlapped the invisible controls, but I could still see them in the dialog editor (hint: if you ever discover you are creating physically overlapping controls, rethink what you are doing. There is always a better way. I'll trade off more complex code for ease of long-term maintenance any day!

When I needed to create an Edit control, I had a variable, c_EditPrototype, which got the value of the edit control that I put on the dialog. I wrote something along these lines:

void CMyForm::CreateEdit(int n) { // Create an edit control on line n of the listbox CRect r; c_EditPrototype.GetWindowRect(&r); CRect line; c_Data.GetItemRect(&line); int left = ...complex computation of left edge of edit control; CRect edit(left, line.top, left + r.Width(), line.top + r.Height()); CEdit * ctl = new CEdit; ctl->Create(styles, edit, &c_Data, controlid++); ...lots more stuff here, including saving the CEdit * for later deletion }

One key insight here is that I did not have to worry about the font size or screen resolution. The edit control prototype was the width desired (all edit controls would be the same width), the height was automatically set when the dialog was created, taking into account the font size, resolution, etc., so all I had to do was copy them.

The buttons were a bit trickier; what I did was create a button with no caption text. This gave me my minimum width; I then added GetTextExtent pixels to that to get the new desired size.

Bottom line, be lazy. Let the system do as much for you as it can.

If I had wanted a caption, I would have stored the caption in the STRINGTABLE and loaded it dynamically. I would never hard-code English text into a product program. If I wanted a button with a non-language caption, I would create it as

c_Button.Create(_T("<<"), BS_PUSHBUTTON, r, this, IDC_REVERSE);

Note the use of _T( ) around the string constant so it is Unicode-aware.

Positioning

Sometimes it is easy. Suppose I need to create a custom control centered in a form, just above another control. We have a desired width and height.

CRect client; GetClientRect(&client); CRect below; c_WhateverControl.GetWindowRect(&below); // the control below the one we want ScreenToClient(&below); // convert to client coordinates gap = 2 * ::GetSystemMetrics(SM_CYEDGE); // allow nice gap between int left = (client.Width() - width) / 2; CRect ctlrect(left, below.top - height - gap, left + width, below.top - gap, this, ctlid);

You can compute all the geometry you want, but if constants appear in any context other than multipliers or divisors, you are in trouble. Note that I even compute the gap based on the screen driver's opinion of the height of the ideal 3-D horizontal edge.

If you know where you want it, and how big it will be, you can simply create, in the dialog editor, a static control, such as a frame or rectangle. Mark it as invisible. Change its ID from IDC_STATIC to something useful. Create a control variable for it. For example, I might call it IDC_CUSTOM and the variable c_CustomFrame.

CRect w; c_CustomFrame.GetWindowRect(&w); ScreenToClient(&w); c_WhateverControl.Create(styles, w, this, ctlid); c_CustomFrame.DestroyWindow(); // no longer need frame

Note that there is no computation of the size; the size has already been computed for me by the dialog handler when it instantiated the static control, and I am creating a custom control in the same area.

Severity: serious

It is a common practice to want to rearrange controls on a dialog at run time, for example, when the dialog is resized. An unfortunate characteristic of such code is that it tends to include hardwired constants, which is almost always an unrecoverable flaw.

This is because dialogs are always expressed in terms of Dialog Box Units, which are an abstract coordinate system. When a dialog is instantiated, the system "realizes" the dialog box by computing a function based on the default system font. From that point on, everything is in terms of pixels. For a very high resolution (e.g., 1600 ×1200) the dialog may take many more pixels than if the display is running at some lower (e.g., 800 × 600), because it tries to keep an approximately same-size-on-the-screen effect of a given dialog. The downside is that any attempt to use a hardwired constant is doomed from the start..

The proper method is to use one of the basic resolution-sensitive parameters of the system to scale the dimensions properly. Two of the key parameters are the border size and the resize border size. For example, I might choose to separate a set of buttons by a multiple of the resize border size. In a very high resolution, this size may be doubled over what it is at a lower resolution. Useful computations I include in my positioning code might be

UINT vsep = 3 * ::GetSystemMetrics(SM_CXBORDER); UINT colsep = 2 * ::GetSystemMetrics(SM_CXEDGE);

The most useful values I have found for ::GetSystemMetrics that are handy to know about are shown in the table below. However, there are many other parameters which can be interesting.

SM_CXBORDER

SM_CYBORDER Width and height, in pixels, of a window border. This is equivalent to the SM_CXEDGE value for windows with the 3-D look. SM_CXEDGE

SM_CYEDGE Dimensions, in pixels, of a 3-D border. These are the 3-D counterparts of SM_CXBORDER and SM_CYBORDER SM_CXFIXEDFRAME,

SM_CYFIXEDFRAME Thickness, in pixels, of the frame around the perimeter of a window that has a caption but is not sizable. SM_CXFIXEDFRAME is the heightof the horizontal border and SM_CYFIXEDFRAME is the width of the vertical border. SM_CXSIZEFRAME,

SM_CYSIZEFRAME Thickness, in pixels, of the sizing border around the perimeter of a window that can be resized. SM_CXSIZEFRAME is the width of the horizontal border, and SM_CYSIZEFRAME is the height of the vertical border. SM_CXHTHUMB Width, in pixels, of the thumb box in a horizontal scroll bar. SM_CYVTHUMB Height, in pixels, of the thumb box in a vertical scroll bar. SM_CYCAPTION Height, in pixels, of a normal caption area. SM_CXVSCROLL,

SM_CYVSCROLL Width, in pixels, of a vertical scroll bar; and height, in pixels, of the arrow bitmap on a vertical scroll bar.

In those rare cases where you may need to create controls dynamically, hardwiring any integer value for the width or height is uniformly a blunder. I have found the most convenient way to create controls dynamically is to lay down a "prototype" control on the dialog. I mark this control as invisible, and create a control member variable for it. Then, for example, if I need to create a control at runtime I use the dimensions of this control to determine the dimensions of the control I want to create, most often the height.

CStatic c_PrototypeStatic; // declared in the class by ClassWizard

//...create the control at position x,y holding text 'caption' CRect r; c_PrototypeStatic.GetWindowRect(&r); ScreenToClient(&r); CClientDC dc(&c_PrototypeStatic); int width = dc.GetTextExtent(caption).cx; width += 2 * ::GetSystemMetrics(SM_CXEDGE); r.right = r.left + width; CRect w(x, y, x + r.Width(), y + r.Height()); ...Create(caption, SS_LEFT, w, this, IDC_STATIC);

Note that I did not show anything to the left of the Create method because there are lots of ways I could create a window as simply calling

CStatic * wnd = new CStatic; wnd->Create(...);

or have an array

wnd[i] = new CStatic; wnd[i]->Create(...);

or have a variable declared in the class and write

c_MyLabel.Create(...);

Here's another trick I use a lot: I need, for whatever reason, to create a window at runtime at a specific location on a dialog. Often this is a custom control of some sort. To get it properly positioned on the dialog, I do not ever rely on coordinates that I "wire in". Instead, I place an invisible CStatic control with an I