MSDN Documentation Errors and Omissions
|
|
Home |
 |
Back To Tips Page |
|
|
MSDN Documentation Errors and Omissions |
|
This summarizes a number of errors and omissions in the MSDN documentation. Do you know of one? I'll add it here, and even give you credit for it. Send your bug reports to newcomer@flounder.com. This also mentions many methods that are missing in the MFC classes.
I am not the only one to have discovered these problems; see, for example, this note from Doug Harrison.
(Note that leading _ and __ characters are ignored in the alphabetization. MFC methods are indexed under their class name)
| A | B | C | D | E | F | G | H | I | J | K | L | M |
| N | O | P | Q | R | S | T | U | V | W | X | Y | Z |
C0..9 |
CA |
CB |
CC |
CD |
CE |
CF |
CH |
CI |
CL |
CM |
CO |
CP |
CR |
CS |
CT |
CW |
LABEL (Asseumbler directive) (8-Nov-09)
SQLValidDSN (6-Jan-09)
Does not state if the Slim Reader/Writer lock can be placed in shared memory (shared segment DLL, shared segment executable, or memory-mapped file) and shared across process boundaries.
Does not state what happens if a thread that has an SRW lock terminates without releasing the lock.
Does not state if this is done as a spin lock or a thread-blocking lock.
Does not state if there is any guaranteed sequencing, or that shared accesses can indefinitely starve exclusive requests, or what.
From inspection of the code: This is a spin lock with no kernel call involved. Therefore, it should not be used to lock a region for any significant length of time. All waiting threads are in a tight loop spinning on the lock. There is no guaranteed sequencing of threads when the lock is released. It can be placed in shared memory shared between processes.
Does not state if the Slim Reader/Writer lock can be placed in shared memory (shared segment DLL, shared segment executable, or memory-mapped file) and shared across process boundaries.
Does not state what happens if a thread that has an SRW lock terminates without releasing the lock.
Does not state if this is done as a spin lock or a thread-blocking lock.
Does not state if there is any guaranteed sequencing, or that shared access can indefinitely starve exclusive requests, or what.
From inspection of the code: This is a spin lock with no kernel call involved. Therefore, it should not be used to lock a region for any significant length of time. All waiting threads are in a tight loop spinning on the lock. There is no guaranteed sequencing of threads when the lock is released. It can be placed in shared memory shared between processes.
This documentation confusingly states
Throws an invalid argument exception.
The correct statement is
Throws a CInvalidArgException * exception.
NOTE: DO NOT CONFUSE THIS WITH HeapAlloc, WHICH IS DIFFERENT!
The AllocateHeap API describing what should be a typedef to a function.
The correct definition is
The AllocateHeap function is a member of the SECPKG_DLL_FUNCTIONS structure and specifies a pointer to the function to be called to allocate memory. The memory will later be freed by the function specified by the FreeHeap member of the SECPKG_DLL_FUNCTIONS structure
typedef PVOID (NTAPI LSA_ALLOCATE_LSA_HEAP) (__in ULOMG Length);
Note that because the function calling convention is NTAPI, ordinary C functions like malloc or new could not be specified.
Note that the absolutely essential calling convention type is not specified! Note also that the input length is a ULONG, not the expected SIZE_T.
See also FreeHeap.
The documentation as delivered is useless. At least the MSDN documentation online is up-to-date. But it is deeply flawed. For example, wouldn't it make sense to tell how to free such pages? Perhaps a hyperlink to what might be the corresponding function, FreeUserPhysicalPages? Would it be asking too much to have such simple See-Also references? There is also no hyperlink to AllocateUserPhysicalPages.
This entry claims it is an alphabetical function reference to the C library, but it is a nearly-completely-empty page, except for stating what it is not.
I put this option in some VS2005 projects. When I moved to VS2008, I find that VS2008 doesn't have this feature, doesn't support it, issues warnings if it is present, has no interface for removing it, does not allow it to be hand-edited from the command line, and I cannot find in the text .sln or .vcproj files anything that says /ANALYZE. So I get annoying warnings. The means of removing this option are not documented, but it turns out that you have to look for a line referencing "PreFast" and delete it. This is, of course, obvious. Right.
It would have been nice to have something meaningful in the description of _ATL_VER, such as the following table. Note that none of this is helped by the total inconsistency where some values are expressed in decimal and some in hex; EVERY symbol that has a "version" meaning shall have all its version history displayed. No exceptions. Ever.
| _ATL_VER | Product release |
| ? | Visual Studio 4.0 |
| ? | Visual Studio 4.1 |
| ? | Visual Studio 4.2 |
| 0x0200 ‡ | Visual Studio 5.0 |
| 0x0300 | Visual Studio 6.0 |
| 0x0700 § | Visual Studio .NET 2002 |
| 0x0710 | Visual Studio .NET 2003 |
| 0x0800 | Visual Studio 2005 |
| 0x0900 § | Visual Studio 2008 |
§ A Tip of the Flounder Fin to Giovanni Dicanio for this information
§ A Tip of the Flounder Fin to David Wilkinson for this information
‡ A Tip of the Flounder Fin to David Scambler for this information
The ASSERT macro is missing an absolutely critical piece of information in its Remarks section:
The ASSERT macro will reset the value found by ::GetLastError to 0 (ERROR_SUCCESS). Therefore, if the last error value is needed, it must be obtained before the ASSERT macro appears, because otherwise, in debug mode, this critical piece of information will be lost. For example, note the following two incorrect examples and the correct example which follows them
BOOL result = AnyAPICall(...);
if(!result)
{
ASSERT(FALSE);
MyReportError(GetLastError()); // WILL ALWAYS REPORT ERROR_SUCCESS IN DEBUG MODE!
}
BOOL result = AnyAPICall(...);
ASSERT(result);
if(!result)
{
MyReportError(GetLastError()); // WILL ALWAYS REPORT ERROR_SUCCESS IN DEBUG MODE!
}
Correct code:
BOOL result = AnyAPICall(...);
if(!result)
{
DWORD error = GetLastError();
ASSERT(FALSE);
MyReprotError(error);
}
The same problem exists with VERIFY.
If both AVIRIFF.h and vfw.h are included in a build, this symbol will produce a "duplicate macro definition" warning because the files are poorly-written, interacting, have an unfortunate sequentiality on their inclusion caused by hopelessly inadequate management of the header files, and just generally represent A Prime Example Of Worst Practice,
If both files are included, vfw.h must be included first. For details see AVIRIFF.h,
If both AVIRIFF.h and vfw.h are included in a build, this symbol will produce a "duplicate macro definition" warning because the files are poorly-written, interacting, have an unfortunate sequentiality on their inclusion caused by hopelessly inadequate management of the header files, and just generally represent A Prime Example Of Worst Practice,
If both files are included, vfw.h must be included first. For details see AVIRIFF.h,
This symbol, which is defined for the AVIOLDINDEX structure in vfw.h, is not documented in the AVI documentation. A search of the MSDN documentation does not reveal any documentation on it. Yet it is found in existing AVI files. What are we to make of this?
The description AVIERR_NOCOMPRESSOR might state something about what had gone wrong. For example.
"It is not valid to assume that any particular compressor is installed. Either a user interface should be provided that lets the user select from a list of installed compressor types, or the compressor type should be checked for existence. The ICInfo API can be used to validate that a compressor type is valid, and it also allows for the enumeration of all the compressor types currently installed."
See Also:
ICInfo, ...
To find a list of the compressor types, I did
for(int i = 0; ; i++)
{
ICINFO info = { sizeof(ICINFO) };
if(!ICInfo(0, i, &info))
break;
// display the info.fccType/info.fccHandler information
}
To validate the compressor type, I did
BOOL IsValidVideoCompressor(DWORD codec)
{
ICINFO info = {sizeof(ICINFO) };
if(!ICInfo(mmioFOURCC('V', 'I', 'D', 'C'), codec, &info))
return FALSE;
return TRUE;
}
The _avioldindex_entry.dwFlags has, in existing files, a flag value 0x00000002 which appears in real files but it is not documented. It turns out it is the AVIIF_TWOCC flag which appears in the header file but is not documented. Note that if a flag has been made obsolete, the documentation must state "This flag is now obsolete, as of <version number here>". That way, we know we can ignore its existence, instead of wondering why it is not documented and whether or not we should be writing it out.
The peNew field is described as
Specifies an array of PALETTEENTRY structures, of size bNumEntries.
The correct documentation is
This array contains the number of PALETTEENTRY structures specified as bNumEntries, or, if bNumEntries is 0, the array will contain 256 PALETTEENTRY structures.
This header file does not contain nor is referenced by adequate documentation. Due to fundamental design errors in the creation of this file and the related vfw.h file, you can get serious compilation errors if you include them in the wrong order. It needs to have this information added:
"If this file is included with vfw.h, then vfw.h must be included first. Due to an error in the way these files were constructed, AVIRIFF.h defines symbols which are either not defined in AVIRIFF.h or would result in "macro redefinition" errors if AVIRIFF.h is included first. This is due to erroneous attempts to duplicate symbols instead of using a common header file that includes all necessary information."
It generates an error of duplicate macro definitions. The sad thing is that the conditionals could work both ways, and it is worth pointing out that the definitions do not need to be different:
AVIRIFF.h line 192 (VS2008)
#ifndef AVIIF_LIST #define AVIIF_LIST 0x00000001 #define AVIIF_KEYFRAME 0x00000010 #endif
vfw.h
#define AVIIF_LIST 0x00000001L #define AVIIF_TWOCC 0x00000002L #define AVIIF_KEYFRAME 0x00000010L
Note also that the AVIIF_TWOCC flag is not defined in AVIRIFF.h, but is defined in vfw.h. Why do we have two conflicting files, anyway?
Note that the AVIIF_TWOCC flag is not documented in the VS2008 documentation.
The documentation for AVISTREAMHEADER appears to be missing from the VS2008 documentation.
The 'strh' data structure is said by the (non-VS2008) AVI documentation as having its data element
"consists of of an AVISTREAMHEADER structure"
But this is not correct. The format of a strh structure is
'strh' len data
but the documentation would suggest that the data portion is the AVISTREAMHEADER. This is not true. The strh chunk is itself an AVISTREAMHEADER. It is not even a complete one, either.
For example, the AVISTREAMHEADER structure is 64 bytes long. The cb field is specified as counting the number of bytes not including the 'strh' (FOURCC fcc) and length (DWORD cb) fields, which is consistent with the layout of the RIFF chunk.
64 - (sizeof(FOURCC) + sizeof(DWORD) => 64 - 8 => 56.
Even accounting for the first two fields, the 'fcc' and 'cb' fields, which actually overlap the AVI header information (so the data part actually starts at the fccType field), I have seen AVI files in which only 48 bytes of data are provided, that is, the rcFrame data is completely missing. This fact is not actually documented in the MSDN online AVI documentation, which is therefore misleading.
Are we at all surprised by this?
The correct documentation would add
"When the AVISTREAMHEADER structure appears in a RIFF file, parts of the structure overlap the chunk type and length information of the RIFF data chunk. Therefore, the structure offset is slightly skewed relative to the start of the data component of the RIFF chunk, as shown in the table below."
| data field offset | structure offset | Field | Description |
| -8 | 0 | FOURCC fcc; | mmioFOURCC('s', 't', 'r', 'h'); overlaps with the chunk ID of the RIFF file |
| -4 | 4 | DWORD cb; | Count of bytes following the cb field; overlaps with the chunk length of the 'strh' record. |
| 0 | 8 | FOURCC fccType | Type of information |
| 4 | 12 | FOURCC fccHandler | Codec handler for the data in the stream |
| 8 | 16 | DWORD dwFlags | Flag bits |
| 12 | 20 | WORD wPriority | (as in existing documentation) |
| 14 | 22 | WORD wLanguage | (as in existing documentation) |
| 16 | 24 | DWORD dwInitialFrames | Number of frames preceding the frames in this stream. |
| 20 | 28 | DWORD dwScale | (as in existing documentation) |
| 24 | 32 | DWORD dwRate | (as in existing documentation) |
| 28 | 36 | DWORD dwScale | (as in existing documentation) |
| 32 | 40 | DWORD dwStart | (as in existing documentation) |
| 36 | 44 | DWORD dwLength | # of frames in this stream |
| 40 | 48 | DWORD dwSuggestedBufferSize | (as in existing documentation) Also, why is there no guidance as to how to set this (e.g., "if 0 is provided, a buffer size appropriate for the data will be used", if that is how it works) |
| 44 | 52 | DWORD dwQuality | One of ICQUALITY_LOW (0), ICQUALITY_HIGH (10000), or ICQUALITY_DEFAULT ((DWORD)-1) |
| 48 | 56 | DWORD dwSampleSize | |
| struct { | When this structure appears in a RIFF data 'strh' chunk, the following fields may be absent. Therefore, you must check the cb field and not attempt to use fields which may not be present. To attempt to use them will result in getting the RIFF file contents out-of-sync with the actual information in the file, leading to program failure. | ||
| 48 | 56 | short left; | (as in existing documentation) |
| 50 | 58 | short top; | (as in existing documentation) |
| 52 | 60 | short right; | (as in existing documentation) |
| 54 | 62 | short bottom; | (as in existing documentation) |
| } rcFrame; | |||
Note that there is no documented guidance on how to fill in many of these fields.
This is a typical example of the vague and confusing documentation which is becoming so common.
It states "The handler for writing AVI files does not accept format changes". What are we to make of this? That you cannot call this API for AVI files? Then why does it exist? There is no way to reconcile this statement with the existence of the API itself!
For example, the lpFormat parameter is said to be a "Pointer to a structure containing the new format". Whatever that means. A code example shows that it is a pointer to a BITMAPINFO structure, which none of the AVI documentation even suggests. How it could possibly be evident to anyone reading the documentation that this is the correct object to point to is beyond comprehension. Nothing I have found suggests what this parameter should point to, or what possible types these values could be.
In the Remarks section, the error code AVI_BADFORMAT should be specified as one of the return values, and one of the explanations should be that "This may indicate that the bits-per-pixel established for the stream and set in the BITMAPINFO structure are incompatible with the codec selected."
The documentation for this macro states (correctly) that it has no meaning in Win32. However, it still appears in various examples, and its usage must be expunged in all cases.
The Remarks section says that _beginthread returns 1L if there is an error. This is incorrect, as can be readily demonstrated by trying to create a few hundred threads. When _beginthread fails, it returns (uintptr_t)–1L.
These calls are unbelievably irresponsibly documented in the latest documentation. The erroneous documentation is
uintptr_t _beginthread( void(*start_address )( void * ), unsigned stack_size, void *arglist ); uintptr_t _beginthreadex( void *security, unsigned stack_size, unsigned (*start_address)( void * ), void *arglist, unsigned initflag, unsigned *thrdaddr );This documentation ignores the fact that there is an explicit linkage type required. In previous versions of the documentation, the correct documentation was given as
uintptr_t _beginthread( void( __cdecl *start_address )( void * ), unsigned stack_size, void *arglist ); uintptr_t _beginthreadex( void *security, unsigned stack_size, unsigned ( __stdcall *start_address)( void * ), void *arglist, unsigned initflag, unsigned *thrdaddr );
Note that these both explicitly state, as part of the syntax of the prototype, the fact that there is a linkage type on the function.
Due to what appears to be either terminal laziness or a complete misunderstanding of how readers use documentation, when managed code added the _clrcall linkage, the linkage type was eliminated from the specification and moved to the commentary code that follows. The correct documentation in modern terms should have been the form shown below.
uintptr_t _beginthread( // NATIVE CODE void( __cdecl *start_address )( void * ), unsigned stack_size, void *arglist );
uintptr_t _beginthread( // MANAGED CODE void( __clrcall *start_address )( void * ), unsigned stack_size, void *arglist ); uintptr_t _beginthreadex( // NATIVE CODE void *security, unsigned stack_size, unsigned ( __stdcall *start_address)( void * ), void *arglist, unsigned initflag, unsigned *thrdaddr );
uintptr_t _beginthreadex( // MANAGED CODE void *security, unsigned stack_size, unsigned ( __clrcall *start_address)( void * ), void *arglist, unsigned initflag, unsigned *thrdaddr );
This is the simple and obvious way to document it correctly. There is NO EXCUSE for the current misleading, erroneous, confusing, and ultimately incorrect form of the prototypes. I have witnessed several failures caused by people expecting the prototypes to accurately reflect the actual prototypes, not be some kind of lazy documenter's excuse for writing poor documentation. Saving a few lines of text and/or a few tens of bytes to produce the incredibly poor documentation we now have for these functions is inexcusable.
This illustrates one of the singular poorest styles of thread coding that can possibly exist! It calls _endthread to terminate the thread. This is a practice so egregiously poor that it must never, ever be encouraged; in fact, the documentation of ExitThread and _endthread and _endthreadex and AfxEndThread should carry large warning labels:
WARNING: THIS FUNCTION SHOULD NEVER BE USED IN ANY REAL PROGRAM! TO CALL IT OPENS THE POSSIBILITY OF SERIOUS PROGRAM MALFUNCTION, LOST STORAGE, AND GENERALLY THE POTENTIAL FOR CATASTROPHIC FAILURE! THE ONLY WAY TO EXIT A THREAD IS TO RETURN FROM THE TOP-LEVEL THREAD FUNCTION!
Even the simplest cases fail catastrophically. Consider
UINT ThreadFunc(LPVOID p)
{
std::vector<int> values;
.... thread code, does lots of values.push_back calls
_endthreadex(0);
return 0;
}
When _endthreadex is called (and this presumes that the thread was created with _beginthreadex, and this should not actually be of any concern to the thread!), the thread terminates. Did you see the destructor for the std::vector getting called? Of course not! So you have a storage leak.
All other scenarios are much worse! The use of _endthread, _endthreadex, AfxEndThread, and ExitThread should be actively discouraged! They should never be used in examples anywhere for any reason whatsoever!
Upon further study, there are so many things wrong with the code that it is unacceptable. Here is a side-by-side comparison of the existing code with the rewrite.
| Original code |
 Revised
code |
| It is worth pointing out that these will normally be found in the stdafx.h file | stdafx.h |
#include <windows.h> #include <process.h> /* _beginthread, _beginthreadex */ #include <stddef.h> #include <stdlib.h> // _threadid #include <conio.h> // _getch |
|
// crt_BEGTHRD.C // compile with: /MT /D "_X86_" /c #include <windows.h> #include <process.h> /* _beginthread, _endthread */ #include <stddef.h> #include <stdlib.h> // _threadid #include <conio.h> // _getch |
#include <stdafx.h> |
void Bounce( void *ch ); void CheckKey( void *dummy ); |
|
| It is well-known that for small intervals this does not produce a good random number selection. The low-order bits are not particularly "random". In fact, the lowest-order bit usually alternates from 0 to 1 | |
/* GetRandom returns a random integer between min and max. */ #define GetRandom( min, max ) ((rand() % (int)(((max) + 1) - (min))) + (min)) |
/* GetRandom returns a random integer between min and max. */ #define GetRandom( min, max ) (( (rand() >> 8) % (int)(((max) + 1) - (min))) + (min)) |
| This should, strictly speaking, be volatile because there is actually no requirement that it be treated consistently with multithreading. In addition, the comment is nonsense; it has nothing to do with video. The name is very non-mnemonic, so I chose to rename it to something meaningful. | |
BOOL repeat = TRUE; /* Global repeat flag and video variable */ |
BOOL running = TRUE; |
| There is no particular reason that this variable should be global | I eliminated the variable because it has no reason to be global |
HANDLE hStdOut; /* Handle for console window */ |
|
| We will need an event object that tells us if all the running threads have terminated. This handle represents that event. | |
HANDLE Complete; |
|
| It is also not clear why this variable needs to be global, but I left it as global | |
CONSOLE_SCREEN_BUFFER_INFO csbi; /* Console information structure */ |
CONSOLE_SCREEN_BUFFER_INFO csbi; /* Console information structure */ |
| It may come as a surprise, but main takes two arguments. In addition, the use of main is very retro, and not Unicode-aware. Time to upgrade it. | |
int main()
{
|
int _tmain(int argc, _TCHAR* argv[])
{
UNREFERENCED_PARAMETER(argc)
UNREFERENCED_PARAMETER(argv)
|
| There is no need for this variable here because of the restructuring. | |
CHAR ch = 'A'; |
|
| This can be a local variable; anyone who needs it can ask for it again. | |
hStdOut = GetStdHandle( STD_OUTPUT_HANDLE ); |
HANDLE hStdOut = GetStdHandle( STD_OUTPUT_HANDLE ); |
/* Get display screen's text row and column information. */ GetConsoleScreenBufferInfo( hStdOut, &csbi ); |
/* Get display screen's text row and column information. */ GetConsoleScreenBufferInfo( hStdOut, &csbi ); |
| This is nonsense. The main thread should be the controlling thread. In particular, this establishes a bad pattern because it suggests that in a GUI app that there should be a secondary thread which has a dialog that has a "Cancel" button, which is of course a completely flaming disaster. Therefore, good patterns should be established by not removing the main thread of control from the main thread. So the main thread will wait for the input from the keyboard. | The key scanning is left in the main thread, and therefore the thread launch is eliminated. |
/* Launch CheckKey thread to check for terminating keystroke. */
_beginthread( CheckKey, 0, NULL );
|
|
| The loop that creates Bounce threads is moved to a secondary thread | |
/* Loop until CheckKey terminates program. */
while( repeat )
{
/* On first loops, launch character threads. */
_beginthread( Bounce, 0, (void *) (ch++) );
/* Wait one second between loops. */
Sleep( 1000L );
}
|
UINT id; HANDLE launch = (HANDLE)_beginthreadex(NULL, 0, Launcher, NULL, 0, &id); |
| The main thread now has responsibility for waiting for the indication that a key has been struck, and if so, it sets the running flag to FALSE | |
_getch(); running = FALSE; |
|
| Note that we will wait for this controlling thread to terminate. We cannot simply exit, because the ExitProcess will force thread termination. In a real application, the threads might have responsibility for closing files, updating databases, or taking other overt actions that must run to completion before the thread terminates, and in the original example, these threads would be killed, very likely before they completed their actions, leaving data in some inconsistent and potentially corrupt state. | |
WaitForSingleObject(launch, INFINITE); CloseHandle(launch); |
|
| Note that although the return type of main is int, the original code has no return statement, meaning that it will not compile error-free. | |
return 0; |
|
} // main |
} // _tmain |
| The comment about _endthread implied is nonsensical; it suggests that _endthread would ever have made sense. | The CheckKey function is omitted entirely. |
/* CheckKey - Thread to wait for a keystroke, then clear repeat flag. */
void CheckKey( void *dummy )
{
_getch();
repeat = 0; /* _endthread implied */
}
|
|
| It is not clear why the retro char data type is being used here. Also, it is worth documenting what is really happening | |
/* Bounce - Thread to create and and control a colored letter that moves * around on the screen. * * Params: ch - the letter to be moved */ |
/* Bounce - Thread to create and and control a colored letter that moves * around on the screen. * * Params: (LPVOID)(TCHAR)ch - the letter to be moved */ |
| Get rid of the retro char type. There is no sane reason to express a space as a hex constant, so a space literal is used. We need the stdout handle here, so instead of using a global variable, we just obtain it as needed. | |
void Bounce( void *ch )
{
/* Generate letter and color attribute from thread argument. */
char blankcell = 0x20;
char blockcell = (char) ch;
BOOL first = TRUE;
|
void Bounce( void *ch )
{
/* Generate letter and color attribute from thread argument. */
_TCHAR blankcell = _T(' ');
_TCHAR blockcell = (_TCHAR) ch;
BOOL first = TRUE;
HANDLE hStdOut = GetStdHandle( STD_OUTPUT_HANDLE );
|
| It is considered poor style to use commas in declaration lists. | |
COORD oldcoord, newcoord; |
COORD oldcoord; COORD newcoord; |
| Change the name of the repeat variable to something that makes sense | |
DWORD result;
/* Seed random number generator and get initial location. */
srand( _threadid );
newcoord.X = GetRandom( 0, csbi.dwSize.X - 1 );
newcoord.Y = GetRandom( 0, csbi.dwSize.Y - 1 );
while( repeat )
{
/* Pause between loops. */
Sleep( 100L );
|
DWORD result;
/* Seed random number generator and get initial location. */
srand( _threadid );
newcoord.X = GetRandom( 0, csbi.dwSize.X - 1 );
newcoord.Y = GetRandom( 0, csbi.dwSize.Y - 1 );
while( running )
{
/* Pause between loops. */
Sleep( 100L );
|
| No change in the loop. However, the indentation and vertical spacing before WriteConsoleOutputCharacter of the blockcell is changed to make it obvious that it is not part of the else statement. | |
/* Blank out our old position on the screen, and draw new letter. */
if( first )
first = FALSE;
else
WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result );
WriteConsoleOutputCharacter( hStdOut, &blockcell, 1, newcoord, &result );
/* Increment the coordinate for next placement of the block. */
oldcoord.X = newcoord.X;
oldcoord.Y = newcoord.Y;
newcoord.X += GetRandom( -1, 1 );
newcoord.Y += GetRandom( -1, 1 );
/* Correct placement (and beep) if about to go off the screen. */
if( newcoord.X < 0 )
newcoord.X = 1;
else if( newcoord.X == csbi.dwSize.X )
newcoord.X = csbi.dwSize.X - 2;
else if( newcoord.Y < 0 )
newcoord.Y = 1;
else if( newcoord.Y == csbi.dwSize.Y )
newcoord.Y = csbi.dwSize.Y - 2;
/* If not at a screen border, continue, otherwise beep. */
else
continue;
|
/* Blank out our old position on the screen, and draw new letter. */
if( first )
first = FALSE;
else
WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result );
WriteConsoleOutputCharacter( hStdOut, &blockcell, 1, newcoord, &result );
/* Increment the coordinate for next placement of the block. */
oldcoord.X = newcoord.X;
oldcoord.Y = newcoord.Y;
newcoord.X += GetRandom( -1, 1 );
newcoord.Y += GetRandom( -1, 1 );
/* Correct placement (and beep) if about to go off the screen. */
if( newcoord.X < 0 )
newcoord.X = 1;
else if( newcoord.X == csbi.dwSize.X )
newcoord.X = csbi.dwSize.X - 2;
else if( newcoord.Y < 0 )
newcoord.Y = 1;
else if( newcoord.Y == csbi.dwSize.Y )
newcoord.Y = csbi.dwSize.Y - 2;
/* If not at a screen border, continue, otherwise beep. */
else
continue;
|
| Given that the character is already in blockcell, there is no reason to re-cast the input parameter again | |
Beep( ((char) ch - 'A') * 100, 175 );
}
|
Beep( (blockcell - 'A') * 100, 175 );
}
|
| Using _endthread is unnecessary because it suggests that it could ever possibly make sense. It is not "redundant" because "it is only going to _endthread when it returns". This presumes that we know that this thread was created by _beginthread, and if we change its creation to _beginthreadex, this code is wrong. See the discussion earlier about C++ and destructors. It is essential for correct and robust behavior that _endthread never be called. | The problem is
that we have to notify the caller that the thread has terminated.
While we could have done an array of HANDLE values, it would not work
correctly because we would be limited to 64 threads, and we don't want to
impose that kind of limitation. Therefore, we keep a count of the
number of active threads. When the count goes to 0, there are no more
threads running, and we can call SetEvent to allow the spawning
thread to resume execution. This thread will exit, and the main thread
is waiting for the launcher to terminate so it can terminate the process. I decided it also looked nicer if the thread erased its character when it completed. |
/* _endthread given to terminate */
_endthread();
}
|
if(!first) WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result ); LONG count = InterlockedDecrement(&threadcount); if(count == 0) SetEvent(Complete); } |
| Instead of launching the
bounce threads in the main application thread, a secondary thread is created
that launches them.
We get rid of the retro char and go for _TCHAR |
|
UINT __stdcall Launcher(LPVOID dummy) { _TCHAR ch = _T('A'); |
|
| It keeps count of the number of threads it launches. As the threads terminate, they decrement this count; when the count goes to 0, the Complete event will be set. We need to create that event | |
Complete = CreateEvent(NULL, TRUE, FALSE, NULL); |
|
|
Before we attempt to create a thread, we increment the count of running
threads. If the creation fails, we decrement that count. In this
case, future attempts to create threads are unlikely to succeed, so we exit
the creation loop and then wait for the threads to complete.
Otherwise, we keep creating threads until the user chooses to stop thread
creation. Note that this also compensates for the error in the documentation that erroneously states that the return value of _beginthread is 1L if there is an error. The original code was erroneous in that in simply kept incremented the ch variable without testing to see if it went out of range. After creating 63 threads, the variable no longer has a valid printable representation. This code has been modified to allow it to remain valid no matter how many threads are created. Note that in one experiment, after slightly over ˝ hour, the program had created 1961 threads before hitting the error condition. |
|
while(running)
{ /* start another thread */
InterlockedIncrement(&threadcount);
uintptr_t result =_beginthread(Bounce, 0, (LPVOID)ch);
if(result == (uintptr_t)-1)
{ /* can't create */
InterlockedDecrement(&threadcount);
break;
} /* can't create */
ch++;
if(!isprint(ch))
ch = _T('A');
Sleep(1000);
} /* start another thread */
|
|
| If we find that running is FALSE, we exit the loop. At this point, we have some number, possibly hundreds, of threads running. We must wait for them all to complete. Note that as each thread terminates, it decrements the count of active threads, threadcount, and when this goes to 0, we know all the threads have terminated, and the Complete event is set. The launcher thread will resume execution, and return, thus terminating this thread. The main thread, having initiated the shutdown, is waiting for this thread to terminate, so it can finally resume execution and exit the program. | |
WaitForSingleObject(Complete, INFINITE); return 0; } |
|
The bmType field is specified as "This member must be zero". Under what conditions? Certainly it shouldn't suggest that this member be initialized to 0 before calling, say, GetObject. Perhaps it should say "When creating a bitmap object with CreateBitmapIndirect, this value must be 0. It will be set to 0 by the GetObject call".
Similarly, the description of bmBits is incorrect. It says "pointer the location of the bit values of the bitmap". This is not quite true. The correct documentation is, :"On a GetObject call, this field is set to NULL. When the BITMAP object is being used to create a bitmap using CreateBitmapIndirect, this is a pointer to the values which will become the bits of the bitmap."
The documentation explains that the width should be multiples of 2 bytes by giving the bizarre explanation
"In other words, bmWidthBytes * 8 must be the next multiple of 16 greater than or equal to the value obtained when the bmWidth member is multiplied by the bmBitsPixel member."
Why is this considered simpler than saying
"If an odd number of bytes is all that is required to hold the bits of a single horizontal scan line, an additional byte will be added so the next scan line starts on a WORD boundary.
I find the stuff in terms of multiples of 16 to be vastly harder to understand and explain than the simple, obvious statement that every scan line starts on a WORD boundary.
The documentation is really out-of-date. It says "The bmBits member must be a long pointer to an array of character (1-byte) values". I don't even know what this means. What is a "long" pointer? We are no longer in 16-bit Windows where we had "near" pointers and "far" (long) pointers, so the notation is nonsensical today. It should be dropped (along with all instances of the keywords FAR and PASCAL, the latter replaced with whatever represents __stdcall). And it very clearly states that the type is an LPVOID, so why it has to be a pointer to 1-byte values doesn't even make the slightest sense. If the bits are 1-bit color, 16-bit color, or 24-bit color, they would not be "an array of bytes", they would be "an array of image data".
It says "each scan is a multiple of 32 bits". What's a "scan"? It should say "The pixels are interpreted as a sequence of horizontal rows of image data, and each row starts on a DWORD-aligned boundary". Note there is a fundamental error here. The bmWidthBytes parameter says that the value must be a multiple of 2 because the bit values of a bitmap form an array that is word aligned" [my emphasis] but the description in the Remarks section says that each scan is a multiple of 32 bits, which means it is DWORD aligned! Which is correct? I have no idea.
In my VS2008 documentation, this structure is omitted from the index. In addition, once found (via "search"), there is no hyperlink to GetObject.
My documentation states
"The pixels on a monochrome device are either black or white. If the corresponding bit in the bitmap is 1, the pixel is turned on (white). If the corresponding bit in the bitmap is 0, the pixel is turned off (black)."
Should say
"A monochrome bitmap is represented by 1-bit pixels. A pixel that has the value 0 is displayed using the first color of the selected color palette and a pixel that has the value 1 is displayed using the second color of the selected color palette. The default palette used has black for color 0 and white for color 1. In 24-bit or 32-bit color mode, a 0 is represented by a black pixel and a 1 is represented by a white pixel. If a paletted DIB is converted to a DDB, the colors used for 0 and 1 are as with rendering into a palleted DC: the 0 bit is displayed as the first color in the palette and the 1 bit is displayed as the second color in the palette."
(4-Aug-08) I was sent the following note, which is quite interesting: This email is reproduced with the permission of the author
This is a real issue. I don't know
how much of the history you know, so I'll go back to the beginning.
The concept of the BITMAP goes back to Windows 1.0. At that time, all
bitmaps were created by a driver: either the display driver or the printer
driver. There were two formats: monochrome, and a color format that matched
current display surface. The format of the bits in a color bitmap was supposed
to be opaque to applications: the BITMAP format was whatever was
convenient for the display or printer driver. Most used something sensible, but
some were very strange indeed.
Because the driver interface required it, scanlines were aligned to a word
(2-byte) boundary, a scan line was not allowed to cross a 64k boundary, and the
top scanline was first. The NT BITMAP code never enforced the 64k
requirement, although Windows 9X kept that limitation until it died.
OS/2 did things a little differently. They created the concept of the DIB
(Device Independent Bitmap) -- a bitmap whose format was defined by contract.
Scan lines in a DIB were padded to a DWORD (4-byte) boundary and were, in
OS/2's inscrutable fashion, defined with the bottom scan line first.
Windows 3.0 absorbed the OS/2 DIB concept (as the BITMAPINFOHEADER), and
the existing bitmap model was renamed Device-Dependent Bitmap. DDBs were
top-down, WORD-aligned, and DIBs were bottom-up, DWORD-aligned.
The code that copied DIBs to and from the screen was contained separately within
each and every display driver, and the code almost universally sucked. Because
of that, and because 99% of the graphics cards in the world used 8-bit-per-pixel
palettized surfaces, application writers shunned DIBs and used DDBs, making the
dangerous assumption that all DDBs were 8-bit.
As time went on, Microsoft provided the DIB engine that let app writers start
using DIBs in real apps without losing performance. Today, it is very rare to
find an app that uses a DDB.
At some point, Microsoft realized that monochrome DDBs and monochrome DIBs
served the same purpose and duplicated code stupidly. They quietly changed the
monochrome DDB definition to DWORD padding, so that a monochrome DDB and
a monochrome DIB could use the same code.
So, you have a very strange melange today. A BITMAP is a DDB. The scan
lines are top-down. A color DDB has word-aligned scans, and a mono DDB has
DWORD-aligned scans. A BITMAPINFOHEADER is a DIB; the scan lines are
bottom-up, and are DWORD-aligned. Making things even more confusing, an
HBITMAP can refer to either one.
No wonder we all want to be web programmers instead of GUI programmers.
--
Tim Roberts, timr@probo.com
Providenza & Boekelheide, Inc.
This contains a very strange statement: "Do not pass to the FromHBITMAP method a GDI bitmap or a GDI palette that is currently (or was previously) selected into a device context"
There is no justification given for this statement, and it raises the question "How do I get the bitmap data filled in?" since the normal way to create a bitmap is to select it into a DC, draw to it, deselect it, and then have a bitmap that has data in it. How, exactly, is the data supposed to be created in the bitmap in the first place? Presumably there is some secret known to the folks at Microsoft as to how we get the data into the bitmap, but it escapes me what it might be.
Like most of the GDI+ documentation, this documentation is incomplete, confusing, and essentially useless.
This does not contain a hyperlink, or a See Also, to BITMAPINFOHEADER or RGBQUAD. In VS2008, the structure is not even indexed! The See Also references are present but totally useless, probably having been pasted in from some vaguely-related API. For example, instead of going someplace useful, the reference to "Structures, Styles, Callbacks, and Message Maps" is utterly meaningless! Under "Structures used by MFC" it lists BITMAPINFO, even though this is a structure of the raw API and has nothing to do with MFC itself. What fool wrote this? At least in previous documentation, there were useful descriptions of these structures written by adults.
The description
The bitmap has a maximum of 216 colors. The biCompression member of the BITMAPINFOHEADER must be BI_BITFIELDS. The bmiColors member contains 3 DWORD color masks which specify the red, green, and blue components, respectively, of each pixel. Bits set in the DWORD mask must be contiguous and should not overlap the bits of another mask. All the bits in the pixel do not have to be used. Each WORD in the array represents a single pixel.
should say
"If the bitmap has a maximum of 216 colors..."
and then something useful should be stated. The rest of the description is incoherent, and I can't figure out what it is talking about. For example, what does it mean by the phrase "all the bits in the pixel do not have to be used"? What does it mean by "each WORD in the array represents a single pixel"? What in the world is this talking about? Could it have been intended to say
"If the bitmap has a maximum of 216 colors..."
Note that 216 is a whole lot more than 216!
I think it is saying is following:
For color depth of 16 bits or fewer, colors may be represented in a compressed format by specifying the biCompression member with the value BI_BITFIELDS. This compression format indicates that the bitmap will be represented by an array of values, where the bits of each WORD represent red, green and blue components of the pixel. For color depth of more than 16 bits, the BI_BITFIELDS indicates that the bitmap will be represented by an array of values, where the bits of each DWORD represent the red, green, and blue components.
The bit fields can be of any width, and the width of the red, green and blue fields are specified by three bit masks in the bmiColors array. bmiColors[0] is a bitmask that indicates the red value, bmiColors[1] is a bitmask that indicates the green value, and bmiColors[2] is a bitmask that indicates the blue value. The fields do not need to be the same size, or in a specific order; the only requirement is that the bit masks specify contiguous, non-overlapping fields. Note that fewer bits than a WORD could be specified.
There should be an example of BI_BITFIELDS somewhere. Note also that there does not seem to be any restriction on the order of the RGB components in the fields, that is, I could use the bitmasks
[0] 0x0000000F [1] 0x000000F0 [2] 0x00000F00
to indicate that the low-order four bits (0xnn0..0xnnF) represent red, the next four bits (0xn0n..0xnFn) represent green, and the next four bits (0x0nn..0xFnn) represent green. But could I also say
[0] 0x00000F00 [1] 0x000000F0 [2] 0x0000000F
or even
[0] 0x000000F0 [1] 0x00000F00 [2] 0x0000000F
as valid representations?
The description
The bitmap has a maximum of 224 colors, and the bmiColors member is NULL. Each 3-byte triplet in the bitmap array represents the relative intensities of blue, green, and red, respectively, of a pixel.
is completely incoherent! The bmiColors member is an ARRAY, and CANNOT POSSIBLY BE NULL! This is like saying
int n[4];
and saying "The value of n is NULL". It is complete and utter nonsense! Didn't anyone actually proofread this documentation before it was released? But why does it say "the bitmap has a maximum of 224 colors"? This sounds like a description of 24-bit color, so in that case, the bitmap has a maximum of 16,777,216 colors! Did it perhaps mean to say 224? Did the writer have a clue that 224 is not the same as 224?
Similarly, by 232 colors did it mean 232? Certainly an earlier section that talks about 256 colors doesn't mean 256!
The description
The biClrUsed member of the BITMAPINFOHEADER structure specifies the number of color indices in the color table that are actually used by the bitmap. If the biClrUsed member is set to zero, the bitmap uses the maximum number of colors corresponding to the value of the biBitCount member.
is completely incoherent. If the biClrUsed member is 0, how does it know what colors to use? It sounds like if the biBitCount is 8, there are a maximum of 256 colors, but what are they? How do we know? What nonsense is this? I have no idea how to read this description or use it!
The discussions that start out "The bitmap is" are nonsensical. They must all start by saying "If the bitmap..." Perhaps a table would encompass it better. But in any case, the current writing is completely incoherent and should be rewritten.
The documentation suggests that the biCompression must be BI_BITFIELDS, which is complete nonsense. Having a technical writer capable of forming a coherent sentence would be a great help in this documentation. This is not a technical problem, it is a problem of having someone who somewhere in their career learned to read and write in coherent sentences, mastered concepts such as the subjunctive mode, and generally knows how to speak and write a language (I do not exclude those for whom English is a second language; often they are better at this than native English speakers who have only one language, because their education forced them to understand fundamental language structures. This documentation would have been incoherent in any language!)
In the VS2008 documentation, this term is not even indexed! It can only be found by "search"
There is a table which states
| 16 | The bitmap has a maximum of 216 colors. If the biCompression member of the BITMAPINFOHEADER is BI_RGB, the bmiColors member of BITMAPINFO is NULL. |
| 24 | The bitmap has a maximum of 224 colors, and the bmiColors member of BITMAPINFO is NULL. |
| 32 | The bitmap has a maximum of 232 colors. If the biCompression member of the BITMAPINFOHEADER is BI_RGB, the bmiColors member of BITMAPINFO is NULL. |
This is completely nonsensical; the bmiColors member is an
array, and thus cannot be NULL. The correct statement in
all cases is "The bmiColors member of the BITMAPINFO structure is
not used"
The documentation of this suggests that the biCompression field is one of a limited value of small integers. However, I have discovered empirically that when the BITMAPINFOHEADER is used as a stream specifier in an AVI file, in the
RIFF('AVI ').LIST('hdrl').LIST('strl').strf
record, the biCompression can be a FOURCC code. I have discovered at least one case where it says 'CRAM' (mmioFOURCC('C', 'R', 'A', 'M') ) as the compression mode. This is undocumented even in the AVI documentation.
Note that messages in the Registered Window Message range cannot be broadcast across desktops, because RegisterWindowMessage returns a unique message code that can be different in every desktop.
The documentation does not state that the pszDisplayName and lpszTitle pointers may be NULL. The description of iImage is most politely defined as "incoherent", since it is not "a variable" but in fact is a member of the structure. The correct documentation is
pszDisplayName
Address of a buffer to receive the display name of the folder selected by the user. The size of this buffer must be at least MAX_PATH characters. If this pointer is NULL, the display name of the folder will not be stored.
lpszTitle
Address of a null-terminated string that is displayed above the tree control view in the dialog box. This string can be used to specify instructions to the user. If this pointer is NULL, no instructions will be displayed. Note that it would be very useful if this bothered to explain if there is a length, or a multiline, restriction, on this string!
iImage
This is set to indicate the image associated with the selected folder. The image is specified as an index to the system image list. And where is the hyperlink so we can learn about what the system image list is, and how we can use it? In the See Also section?
This function can accept a NULL parameter, but the documentation does not specify what happens if NULL is used as an argument. The implementation files frequently use Attach(0) but we cannot guess what this is doing.
The Remarks states that Assign "does a binary copy, which means the entire length of the BSTR is copied, regardless of content". There is absolutely nothing that explains anything about what this gibberish means, nor is there a hyperlink to a description of what a BSTR actually is (for example, this implies that a BSTR, analogous to a CString, has both a "maximum buffer size" and a "current characters used". So there is no reason to expect the reader has a clue as to what this actually means.
There are problems with the examples that are not clear and seem to contradict other documentation.
For example, near the end of the example, the statement
_snwprintf_s(bstrWrapper.GetBSTR(), 100, bstrWrapper.length(), L"changing BSTR");
It is not at all clear why this works. For example, how was the magical value "100" chosen for the length? Why is it meaningful? What is the possible range of values permitted? How do we know the buffer that is returned by GetBSTR points to a memory area that is guaranteed to have 100 characters of space?
It is worth pointing out that this is one of those "theoretical" examples, written by someone who never actually tested the code to see if it works! It doesn't. This doesn't even meet Doug Harrison's "fanciful" criterion; fanciful code may be incomplete and confusing, but I believe it more-or-less works. This never worked.
In the example, the code reads:
// new value into BSTR _snwprintf_s(bstrWrapper.GetBSTR(), 100, bstrWrapper.length(), L"changing BSTR"); wprintf_s(L"bstrWrapper = %s\n", static_cast<wchar_t*>(bstrWrapper)); wprintf_s(L"bstrWrapper2 = %s\n", static_cast<wchar_t*>(bstrWrapper2));
Now, before the _snwprintf, the values are (from the watch windows:)
| Watch Window | ||||||||||||||||||||||||||||||
|
But after the _snwprintf, the values are
| Watch Window | ||||||||||||||||||||||||||||||
|
If we look at the memory dump, the characters shown are as illustrated below (it has decoded the bytes as ANSI characters; apparently there is not a way to tell a memory dump to interpret the characters as Unicode).
| Memory Window |
0x00337470 fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţ 0x00337480 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x00337490 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374A0 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374B0 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374C0 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374D0 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374E0 fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ţţţţţţţţţţţţţţţţ 0x003374F0 fe fe fe fe ee fe ee fe ee fe ee fe ee fe ee fe ţţţţîţîţîţîţîţîţ 0x00337500 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337510 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337520 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337530 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337540 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337550 ee fe ee fe ee fe ee fe ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ 0x00337560 ee fe ee fe ee fe ee fe îţîţîţîţîţîţîţîţ |
It displays 64 characters of UFEFE and 64 characters of UFEEE.
Doesn't this look like free storage? So the code as written does not work! I therefore have no idea what the point of this dysfunctional example is.
The various statements about GetBSTR says that it "affects all _bstr_t objects that share a BSTR". So let's look at those last few lines, commented "new value into BSTR"
_bstr_t bstrWrapper contains "Yet another string" _bstr_t bstrWrapper2 contains "some text"
These are clearly unique strings. Then there is the odd _snwprintf_s call that clearly changes one of them. The resulting state of the strings is
_bstr_t bstrWrapper contains "changing BSTR" _bstr_t bstrWrapper2 contains "some text"
Well, whoopee. Nothing was assigned to bstrWrapper2, so why is interesting that it didn't change? A more informative example would have assigned the same BSTR to two _bstr_t variables, and illustrated that changing one of them changed the other!
Here's a working example of the effects of sharing:
// show non-sharing
BSTR bstr = ::SysAllocString(OLESTR("New Shared String"));
wprintf_s(L"Using GetAddress to show non-sharing\n");
bstrWrapper = "Shared string";
bstrWrapper2 = bstrWrapper;
wprintf_s(L"bstrWrapper = %s\n",
static_cast(bstrWrapper));
wprintf_s(L"bstrWrapper2 = %s\n",
static_cast(bstrWrapper2));
| Output window |
Before assignment:
bstrWrapper = Shared string
bstrWrapper2 = Shared string
|
| Watch Window | ||||||||||||||||||||||||||||||
|
Note that both variables have the same m_Data and m_wstr values, and the m_RefCount is 2.
*bstrWrapper2.GetAddress() = bstr;
wprintf_s(L"bstrWrapper = %s\n",
static_cast(bstrWrapper));
wprintf_s(L"bstrWrapper2 = %s\n",
static_cast(bstrWrapper2));
| Output window |
After assignment
bstrWrapper = Shared string
bstrWrapper2 = New Shared String
|
Note that the assignment created a new copy in bstWrapper2 but had no effect on the contents of bstrWrapper1
| Watch Window | ||||||||||||||||||||||||||||||
|
// show sharing
bstrWrapper = "Shared string";
bstrWrapper2 = bstrWrapper;
wprintf_s(L"bstrWrapper = %s\n",
static_cast(bstrWrapper));
wprintf_s(L"bstrWrapper2 = %s\n",
static_cast(bstrWrapper2));
| Output Window |
Before assignment
bstrWrapper = Shared string
bstrWrapper2 = Shared string
|
| Watch Window | ||||||||||||||||||||||||||||||
|
bstrWrapper2.GetBSTR() = ::SysAllocString(L"New Shared String");
wprintf_s(L"bstrWrapper = %s\n",
static_cast
| Output Window |
After assignment
bstrWrapper = New Shared String
bstrWrapper2 = New Shared String
|
| Watch Window | ||||||||||||||||||||||||||||||
|
Note that in this case, the assignment to one variable, via the GetBSTR function, affects all the variables that share that BSTR.
The one-line summary under "_bstr_t Class" says
GetAddress Points to the BSTR wrapped by a _bstr_t
This is confusing and misleading. The correct documentation is
GetAddress Creates an out reference to a BSTR and sets the length of the BSTR to zero; most commonly used on the left-hand side of an assignment statement.
The first definition, the one that actually is given, makes it confusing to distinguish why GetAddress differs from GetBSTR.
The same confusion occurs at the actual function definition; the Remarks section conspicuously fails to specify that the BSTR is set to have a 0-length. The remarks about the fact that it "affects all _bstr_t objects that share a BSTR" is confusing because there is no example of what this means and there is no hyperlink to any discussion of this sharing.
The badly-broken example showing how GetAddress is used, which is part of the _bstr_t::Assign discussion, shows the method is used on the left-hand-side of an assignment statement, which is not at all an obvious usage from the descriptions. Furthermore, as my example demonstrates, the use of GetAddress does not affect all the BSTRs involved!
The one-line summary under "_bstr_t Class" says
GetBSTR Points to the beginning of the BSTR wrapped by the _bstr_t
It is not clear why anyone cares about the concept of "beginning of the BSTR". The correct documentation would be
GetBSTR Points to the BSTR wrapped by the _bstr_t; used when a BSTR input argument is required.
The same confusion applies in the actual function description. It contains the same gibberish about affecting all _bstr_t objects; there is no hyperlink to a discussion of this nor is there an example that illustrates why this might be a problem. The hyperlink to _bstr_t::Assign seems to contradict this. But my example, which is an extension of the badly-broken _bstr_t::Assign example, demonstrates how this actually works.
The documentation fails to mention how the style bits are grouped and which are mutually exclusive with each other.
The following should at least be stated:
The button styles can be zero or more of the following styles, in combinations. Note that within each category, certain styles are mutually exclusive and at most one of the styles can be used.
| Style | Description |
| Button type | Defines the appearance of the button. Select zero or one of these. If one of these is not specified, BS_PUSHBUTTON is assumed. |
| BS_PUSHBUTTON | A classic pushbutton. |
| BS_DEFPUSHBUTTON | The same as BS_PUSHBUTTON, but it is highlighted as if it were a default pushbutton. Note that in a dialog, changing the style does not establish the default property of the button; a DM_SETDEFID message specifying the handle of the button must also be sent to the dialog. |
| BS_CHECKBOX | A checkbox style button with two states. These are BST_CHECKED and BST_UNCHECKED. The value BST_CHECKED shows a check-mark; the value BST_UNCHECKED does not show a check mark. Clicking on the button sends a button notification, but does not change the state of the button. |
| BS_AUTOCHECKBOX | A checkbox style button with two states. These are BST_CHECKED and BST_UNCHECKED. The value BST_CHECKED shows a check-mark; the value BST_UNCHECKED does not show a check mark. Clicking on the button cycles between these two states. |
| BS_3STATE | A checkbox style button, but one which has three states. The three states are BST_CHECKED, BST_UNCHECKED and BST_INDETERMINATE. The value BST_CHECKED shows a check-mark; the value BST_UNCHECKED does not show a check mark; the value BST_INDETERMINATE shows a dimmed check mark. Clicking on the button sends a button notification, but does not change the state of the button. Note the documentation that says "the dimmed state is typically used to show that a check box has been disabled is nonsense. |
| BS_AUTO3STATE | A checkbox style button, but one which has three states. The three states are BST_CHECKED, BST_UNCHECKED and BST_INDETERMINATE. The value BST_CHECKED shows a check-mark; the value BST_UNCHECKED does not show a check mark; the value BST_INDETERMINATE shows a dimmed check mark. Clicking on the button cycles the button state between BST_UNCHECKED, BST_CHECKED, and BST_INDETERMINATE, in that sequence. |
| BST_RADIOBUTTON | Defines a radiobutton style control. A radiobutton has two states, BST_CHECKED and BST_UNCHECKED. The value BST_CHECKED shows a circle in the radio button indicating that it is in the checked state; the value BST_UNCHECKED shows a radio button without a circle inside it, indicating it is the unchecked state. Clicking a radio button sends a button notification to the parent, but does not change the state of the button or any other radio buttons in the radio button group. |
| BS_AUTORADIOBUTTON | Defines a radiobutton stle control. A radiobutton has two states, BST_CHECKED and BST_UNCHECKED. The value BST_CHECKED shows a circle in the radio button indicating that it is in the checked state; the value BST_UNCHECKED shows a radio button without a circle inside it, indicating it is the unchecked state. Clicking an auto-radiobutton will send a button notification to the parent, even if the button is already in BST_CHECKED state. If the button is in BST_UNCHECKED state, the checkmark will be removed from the button in the radio button group and it will be set to BST_UNCHECKED. |
| BS_OWNERDRAW | The button will be owner-drawn. Drawing will be handled by a WM_DRAWITEM message handler. |
| BS_PUSHBOX | Obsolete. Existed only in 16-bit Windows code. |
| BS_USERBUTTON | Obsolete. Existed only in 16-bit Windows code. |
| BS_GROUPBOX | The button is a group box, a rectangle in which other buttons are displayed. Text associated with this control will always be displayed in the top border of the button. The BS_BOTTOM, BS_CENTER, BS_TOP, BS_LEFT, BS_RIGHT and BS_CENTER styles will determine its alignment. |
| Text alignment relative to button | These styles apply to check boxes and radio buttons, and determine the relationship of the text to the control. |
| BS_LEFTTEXT BS_RIGHTBUTTON |
The text appears to the left of the button, This applies to all button styles except BS_GROUPBOX. Both symbols have the same value. Note that both of these symbols represent two different ways to specify exactly the same style bit. |
| Vertical text alignment within button | Establishes the vertical text alignment within the button. If not specified, a default suitable for the button is chosen. Choose zero or one of the styles below. |
| BS_BOTTOM | Text is vertically aligned with the bottom of the button. Cannot be combined with BS_VCENTER or BS_TOP |
| BS_VCENTER | Text is vertically centered between the top and bottom of the button. Cannot be combined with BS_BOTTOM or BS_TOP |
| BS_TOP | Text is vertically aligned with the top of the button. Cannot be combined with BS_BOTTOM or BS_VCENTER |
| Horizontal text alignment within button | Establishes the text alignment within the button. If not specified, a default suitable for the button is chosen. Choose zero or one of the styles below. |
| BS_LEFT | Text is horizontally aligned to the left of the button rectangle. Cannot be combined with BS_CENTER or BS_RIGHT |
| BS_CENTER | Text is horizontally centered in the button rectangle. Cannot be combined with BS_LEFT or BS_RIGHT |
| BS_RIGHT | Text is horizontally aligned to the right of the button rectangle. Cannot be combined with BS_LEFT or BS_CENTER. |
| Button content | Defines content display. For some button styles, text is always used, and the content selection value is ignored. Choose zero or one of the styles below. |
| BS_TEXT | The button will display text. Incompatible with BS_ICON and BS_BITMAP |
| BS_ICON | The button will display an icon. Incompatible with BS_TEXT and BS_BITMAP |
| BS_BITMAP | The button will display a bitmap. Incompatible with BS_ICON and BS_TEXT |
| Button appearance | Defines border and content display. Choose zero or more of the styles below. |
| BS_MULTILINE | Wraps the button text to multiple lines if the text is too long to fit on a single line within the button rectangle. Note that the button rectangle must be vertically tall enough to hold the text. |
| BS_FLAT | Specifies the button is two-dimensional, and does not use any shading techniques to give a 3-D appearance |
| BS_PUSHLIKE | Modifies a radio button, autoradio button, check box, or autocheckbox to look like a regular button. When the button is in the BST_CHECKED state, it appears to be pressed, and when it is in the BST_UNCHECKED state, it appears as an unpressed button |
| Button behavior | Choose zero or one of the options below. |
| BS_NOTIFY | Enables the sending of BN_DBLCLK, BN_KILLFOCUS and BN_SETFOCUS notifications tot he parent. |
Does not contain a hyperlink to the SBYTE directive. Does not contain a crosslink to the DB directive.
Does not specify that the initializer values can be in the range -128..255. Does not give the syntax for a string, or mention that a string is not implicitly NUL-terminated.
VS2005 will diagnose an error which passed earlier compilers, and issue a message of the form
c:\program files\microsoft visual studio 8\vc\atlmfc\include\afxwin.h(931) : error C2248: 'CObject::CObject' : cannot access private member declared in class 'CObject'
c:\program files\microsoft visual studio 8\vc\atlmfc\include\afx.h(558) : see declaration of 'CObject::CObject'
c:\program files\microsoft visual studio 8\vc\atlmfc\include\afx.h(529) : see declaration of 'CObject'
This diagnostic occurred in the compiler generated function 'CDC::CDC(const CDC &)'
Notice that at no point does this actually refer to your code. Therefore, there is no effective procedure to determine where the error occurred. The error is indicated at the closing brace of class CDC. Shall we say that this error message is less than informative?
To add to the confusion, the "task list" only shows the first line of this file, and does not indicate what module was being compiled at the time the error occurred. This is a grotesque design error in Visual Studio.
So first, you have to go back to the Output window to get the above several lines, and to see what module is being compiled.
If you look at CObject::CObject, you will see the following lines:
// Disable the copy constructor and assignment by default so you will get // compiler errors instead of unexpected behaviour if you pass objects // by value or assign objects. protected: CObject();
Now, while the idea of having the compiler detect serious errors is a Good Idea, having it issue the particular error it does, without any way of finding the source line that caused it, is a Really, Really, Bad Idea.
So how do you find the problem?
I did "binary cut". I went to the end of my source program that was failing and added
#endif // CObject
Then I went approximately halfway back into the file, right before a procedure started, and added
#if 0 // CObject
and rebuilt it. The error still existed. So I knew it happened before that. So I went back to about 1/4 into the file, moved the #if 0 there, and rebuilt The error still appeared. I went back to about 1/8 into the file, moved the #if 0 there, and the error disappeared. So I knew it was somewhere between 1/8 and 1/4 of the distance into the file.
So I moved the #endif to the 1/4 point and compiled again, and no, the error did not occur. So I looked at the functions between the two, and only one used a CDC in any way. So I then moved the #if 0 and #endif to just inside the { } of what was the OnPaint handler, tried again. It compiled. Now, play binary cut with the body of the function. I got to one point where there was a suspicious construct and indeed it seemed to be passing the wrong parameter, which is what triggered the error. I changed it to &dc and the problem went away. The problem was caused by overload resolution if I just specified dc.
Not a fun experience, and more so because the error message is so misleading and provides nothing useful.
There's another cause, for example, I declare
class MyClass {
public:
CStringArray a;
};
I then add either of two declarations:
CArray<MyClass> MFCCollection; std::vector<MyClass> STLCollection;
any attempt to use these classes, such as
STLCollection.clear(); MFCCollection.Add(MyClass());
will generate the following error message
1>c:\program files\microsoft visual studio 8\vc\atlmfc\include\afxcoll.h(593) : error C2248: 'CObject::operator =' : cannot access private member declared in class 'CObject' 1> c:\program files\microsoft visual studio 8\vc\atlmfc\include\afx.h(559) : see declaration of 'CObject::operator =' 1> c:\program files\microsoft visual studio 8\vc\atlmfc\include\afx.h(529) : see declaration of 'CObject' 1> This diagnostic occurred in the compiler generated function 'CStringArray &CStringArray::operator =(const CStringArray &)'
Now, in any intelligently-designed language and compiler, this error message would be associated with the actual cause, which is the use of the class (such as the line that says STLCollection.clear() or MFCCollection.Add(BadClass()) but that would not be any fun. So you are presented with this obscure game.
The real cause is that CStringArray does not have a public assignment operator (it is not clear why CObject has a protected assignment operator!)
The reason is that the operations are trying to assign an object of type BadClass, which means that in order to do a member assignment, it must assign a CStringArray, which is impossible because MFC collections do not have assignment operators.
The solution is to not use a CStringArray, but instead declare the class as
class MyClass {
public:
std::vector<CString> a;
};
Then it will work, because std::vector has a defined assignment operator.
If you encounter this error on a project in VS2008, it is because you have converted this project from earlier versions of VS. There is an undocumented, incompatible change in the specification of the ON_WM_NCHITTEST handler. See CWnd::OnNcHitTest for the details.
There are many reasons I detest the CArchive class, and every time I touch it I find another. The latest is that CArchive::Write wants a void * argument, where in a sane world it would take a const void * argument.
The article KB192570 on asynchronous sockets in multiple threads is a disaster. See my rewrite.
For lParam, it should state that for a non-owner-draw control, or an owner-draw control with CBS_HASSTRINGS, a copy of the string referenced by the LPARAM is stored in the control.
For lParam, it should state that for a non-owner-draw control, or an owner-draw control with CBS_HASSTRINGS, a copy of teh string referenced by the LPARAM is stored in the control.
The return type is specified as CB_ERR if an error occurs, but is not specified for success. Is it CB_OKAY or is it some other random value?
Why does this refer to CGDIObject::GetObject to find out the number of bits when it should reference CBitmap::GetBitmap?
The documentation sucks. It enumerates some hex constants without actually using the symbolic values that should be there. The mask 0x0003, unfortunately, is required, because there is no symbol with a reasonable name like BST_STATE_MASK, but there is no excuse for using 0x0004 in place of BST_PUSHED or 0x0008 in place of BST_FOCUS. In addition, it states that
A 0 indicates the button is unchecked. A 1 indicates the button is checked....A 2 indicates the check state is indeterminate.
This is correct, but slovenly in that it encourages the use of literal values and does not encourage the use of symbolic names. To be quality documentation, it should state
The value after masking is equal to BST_UNCHECKED if the button is unchecked, BST_CHECKED if the button is checked, and BST_INDETERMINATE if the check state is indeterminate (applies only to 3-state check boxes).
This states that values returned are 0, 1 or 2. The correct documentation is
Return value
BST_UNCHECKED if not checked, BST_CHECKED if checked, and BST_INDETERMINATE if it is in the indeterminate state.
This states that values used should be 0, 1 or 2; the correct documentation is
nCheck
State of the check box: BST_UNCHECKED to clear it, BST_CHECKED to set it, and BST_INDETERMINATE if it is in the indeterminate state.
Like most MFC locking primitives, there is a fundamental simple principle to guide the usage of this type:
NEVER, UNDER ANY CIRCUMSTANCES IMAGINABLE, EVER, USE A SYNCHRONIZATION PRIMITIVE DEFINED BY MFC!!!!!!!!!
It's that simple.
The correction to the documentation is to label these classes as "broken, and broken by design, not to be used".
But here's the proof:
A critical section, like a mutex, has "recursive acquisition semantics", that is, if acquired more than once by the same thread, the acquisition merely increases a reference count, and there must be as many releases as acquisitions. The proof is that you can write
CRITICAL_SECTION lock; ::InitializeCriticalSection(&lock); ::EnterCriticalSection(&lock); ::EnterCriticalSection(&lock); ::LeaveCriticalSection(&lock); ::LeaveCriticalSection(&lock); ::DeleteCriticalSection(&lock);
(You may ask why you would lock twice in a row; the above code shows a logical presentation of what can be a complicated execution sequence spanning many functions). But the above code works. Now let's look at CCriticalSection in the same context:
CCriticalSection crit; CSingleLock lock(&crit); lock.Lock(); lock.Lock(); lock.Unlock(); lock.Unlock();
You would think this would work, but it does not and cannot be made to work. Let's look at the lock and unlock code in MFC:
BOOL CSingleLock::Lock(DWORD dwTimeOut /* = INFINITE */)
{
ASSERT(m_pObject != NULL || m_hObject != NULL);
ASSERT(!m_bAcquired);
m_bAcquired = m_pObject->Lock(dwTimeOut);
return m_bAcquired;
}
BOOL CSingleLock::Unlock()
{ ASSERT(m_pObject != NULL);
if (m_bAcquired)
m_bAcquired = !m_pObject->Unlock();
// successfully unlocking means it isn't acquired
return !m_bAcquired;
}
How many fundamental design errors can you spot in this code? Let's see, we can't pass a pointer to it to another thread because the m_bAcquired is not using any form of synchronized access. It assumes that recursive acquisition can never exist, because it assumes on a Lock call that m_bAcquired is FALSE. It is set if the lock is acquired. If it is not set, the Unlock code cannot be executed.
How, exactly, this could ever possibly make sense for a mutex, semaphore, CRITICAL_SECTION or event escapes me. It cannot make sense. For example, and I discovered this years ago, if the Event has a name and is shared between processes, each process will have its own CSingleLock object and the correlation of the m_bAcquired state and the actual state is impossible to maintain.
Had the m_bAcquired variable been eliminated, this code could be made to work, but as it stands, this code is complete crap, and must never be used (unless, of course, you have no interest in the correctness of your code).
Does not exist. Why not?
Does not have a See Also to CDC::GetCurrentBrush, CDC::GetCurrentFont, CDC::GetCurrentPalette, CDC::GetCurrentPen, or CDC::GetCurrentObject.
Does not have a See Also to CDC::GetCurrentBitmap, CDC::GetCurrentFont, CDC::GetCurrentPalette, CDC::GetCurrentPen, or CDC::GetCurrentObject.
Does not have a See Also to CDC::GetCurrentBitmap, CDC::GetCurrentBrush, CDC::GetCurrentPalette, CDC::GetCurrentPen or CDC::GetCurrentObject.
Does not have a See Also to CDC::GetCurrentBitmap, CDC::GetCurrentBrush, CDC::GetCurrentFont, CDC::GetCurrentPalette, or CDC::GetCurrentPen.
Does not have a See Also to CDC::GetCurrentBitmap, CDC::GetCurrentBrush, CDC::GetCurrentFont, CDC::GetCurrentPen or CDC::GetCurrentObject.
Does not have a See Also to CDC::GetCurrentBitmap, CDC::GetCurrentBrush, CDC::GetCurrentFont, CDC::GetCurrentPalette or CDC::GetCurrentObject.
This fails if executed in a DC where the SetWorldTransform has applied a transformation where the values have gone infinite (e.g., a zero-height shear of infinite width). This would not be so bad, except the code has two serious implementation problems:
It creates a VERIFY failure in the debug build
The fix would certainly involve initializing the SIZE value in the function to (0, 0), and removing the VERIFY. The user should be expected to be able to respond properly to such an error. My workaround was
BOOL ok = ::GetTextExtentPoint32(dc->m_hAttribDC, s, (int)s.GetLength(), &sz);
Does not work correctly in GM_ADVANCED with scaling. See DPtoLP
This does not exist, and should. It can be written as
XFORM M; ...set M ::ModifyWorldTransform(dc, &M, mode); // for CDC
or
XFORM M; ...set M ::ModifyWorldTransform(*dc, &M, mode); // for CDC*
because of the implicit CDC::operator(HDC) which will do the correct casting.
The documentation is misleading and confusing. It says
The function assumes that the coordinates for the given region are specified in device units.
But this is not correct. The phrase "device units" suggests that the distance units are in units of MM_TEXT. But it also means the origin and x,y distances will use the coordinate system of MM_TEXT. The correct statement is
The function uses a region that is specified in device units, in the device coordinate space; that is, any transformations applied via SetWorldTransform or other transform-matrix operations, or which use mapping modes other than MM_TEXT, are ignored.
This does not exist, and should. It can be written as
::SetGraphicsMode(dc, GM_ADVANCED); // for CDC
or
::SetGraphicsMode(*dc, GM_ADVANCED); // for CDC*
because of the implicit CDC::operator(HDC) which will do the correct casting.
See also SetGraphicsMode
The documentation is not as thorough as the raw API SetTextCharExtra in that it does not specify that if there is an error, the return value is 0x80000000, or perhaps INT_MIN. It does not specify if nCharExtra can be negative.
This does not exist, and should. It can be written as
XFORM M; ...set M ::SetWorldTransform(dc, &M); // for CDC
or
XFORM M; ...set M ::SetWorldTransform(*dc, &M); // for CDC*
because of the implicit CDC::operator(HDC) which will do the correct casting.
This gives misleading information about how to create a dialog using a dialog ID. For example, it says
Two forms of the Create member are provided for access to the dialog-box resource by either template name or template ID number (for example, IDD_DIALOG1).
This is misleading, because it suggests that using the IDD_ number is actually the correct way to do this. It is not. The example is equally poor, showing
BOOL ret = pDialog->Create(IDD_MYDIALOG, this);
only reinforces this bad idea. The correct documentation would be
Two forms of the Create member are provided for access to the dialog-box resource by either template name or template ID number (for example, CMyDialog::IDD)
and the corrected example should be
BOOL ret = pDialog->Create(CMyDialog::IDD);
Note also that the specification says that the prototype is
virtual BOOL CDialog::Create(LPCTSTR lpszTemplateName, CWnd * pParentWnd = NULL); virtual BOOL CDIalog::Create(UINT nIDTemplate, CWnd * pParentWnd = NULL);
It suggests that the parent window will be used. This is erroneous. The API (see CreateDialog) ignores this parameter and sets the parent to be the main window of the application. This is not documented anywhere. Note that this is not an MFC failure, but a fundamental failure of the underlying API to work as documented.
The documentation also does not have a hyperlink to CDialog::CreateIndirect.
The documentation gives erroneous information. It specifies the methods as
virtual BOOL CreateIndirect(LPCDLGTEMPLATE lpDialogTemplate, CWnd * pParentWnd = NULL, void * lpDialogInit = NULL); virtual BOOL CreateIndirect(HGLOBAL hDialogTemplate, CWnd * pParentWnd = NULL);
The documentation erroneously states that the window specified as the pParentWnd will be the parent of the dialog. This is not true. The parameter is ignored completely, even if it is not NULL, and the parent is always set to the main application window. The documentation lies. Note that this is not an MFC issue, but a fundamental failure of the underlying API, which is erroneously documented.
This example is exceptionally poor, and in fact will not compile as written. The reason is that it uses a new operator on a view, and the constructor of a CView-derived class is protected. No mention of this is made. It is entirely possible this article has not been reviewed since the original 16-bit MFC release.
In addition, this covers the esoteric situation of creating a set of multiple views in an SDI app, and not the common situation of creating a second view in and MDI app. It is poorly conceived and badly written.
The more common case is adding a view to an existing document. My code for doing this will be shown below.
First, create a string to define the view. Suppose I created a Red class for my original application, and therefore had CRedView and CRedDocument. Now I wish to add a Green view. The steps are
IDR_RedViewTYPE \nRed\nRed\n\n\nRed.Document\nRed Document IDR_GreenViewTYPE \nRed\nRed\n\n\nRed.Document\nRed Document
Note that the contents of the string are identical because the document type is still a Red document, we are just getting a Green view of it!
ID_WINDOW_NEWGREENVIEW 'New &Green View'
CMultiDocTemplate * GreenTemplate;
GreenTemplate = NULL;
delete GreenTemplate
CDocument * GetCurrentDocument();
pDocTemplate = new CMultiDocTemplate(IDR_RedViewTYPE,
RUNTIME_CLASS(CMultiViewDoc),
RUNTIME_CLASS(CChildFrame), // custom MDI child frame
RUNTIME_CLASS(CRedView));
if (!pDocTemplate)
return FALSE;
AddDocTemplate(pDocTemplate);
void CMainFrame::OnWindowGreenview()
{
if(GreenTemplate == NULL)
GreenTemplate = new CMultiDocTemplate(IDR_GreenViewTYPE,
RUNTIME_CLASS(CMultiViewDoc),
RUNTIME_CLASS(CChildFrame),
RUNTIME_CLASS(CGreenView));
ASSERT(GreenTemplate != NULL);
if(GreenTemplate == NULL)
return; // internal error should not occur
CMultiViewDoc * doc = (CMultiViewDoc *)GetCurrentDocument();
ASSERT(doc != NULL);
if(doc == NULL)
return; // should not be possible
CFrameWnd * frame = GreenTemplate->CreateNewFrame(doc, NULL);
ASSERT(frame != NULL);
if(frame != NULL)
{ /* frame created */
frame->InitialUpdateFrame(doc, TRUE);
} /* frame created */
}
CDocument * CMainFrame::GetCurrentDocument()
{
CMDIFrameWnd * frame = (CMDIFrameWnd*)AfxGetApp()->m_pMainWnd;
if(frame == NULL)
return NULL;
CMDIChildWnd * child = (CMDIChildWnd *)frame->GetActiveFrame();
if(child == NULL)
return NULL;
CView * view = child->GetActiveView();
if(view == NULL)
return NULL;
CDocument * doc = view->GetDocument();
return doc;
} // CMainFrame::GetCurrentDocument
The documentation misses an important description. The documentation describes only
BOOL GetCueBanner(
LPWSTR lpwText,
int cchText);
but it omits the critical description
CString GetCueBanner();
which returns a CString. See also EM_GETCUEBANNER.
This class cannot be realistically used. There are so many things wrong with the implementation that it is unsalvageable.
For example, you cannot use a CSingleLock across process boundaries, or in a loop. If you leave the scope where you have locked the event, the destructor will call ::SetEvent, even though your intention would be to release the event in some other context. Essentially, this class cannot be used in any real program.
There is an error in the description of the nMaxError parameter; it should read
nMaxError
The maximum number of characters the buffer can hold, including the NUL terminator.
There is no NULL character; NULL is a pointer. The name of the character whose value is 0 is NUL. One L. All that is required to know this is fundamental literacy, since every legitimate book that talks about character values states this explicitly. For example, the character table in The Unicode Standard 3.0, page 336.
The Remarks section also talks about adding "a trailing null". No, it might say, "a trailing null character" but the correct documentation would say "a trailing NUL"
It also states that if "the buffer is too small, the error message may be truncated" (Emphasis added). This means there is a probability the message could be truncated, or perhaps it won't be. So which is it? And why can't I find out how big a buffer I should allocate? If the error message is truncated, will the function return FALSE? Does it call ::SetLastError to indicate ERROR_INSUFFICIENT_BUFFER? This method looks like it was written by summer intern. Perhaps the documentation should state
Note: GetErrorMessage will not copy more than nMaxError - 1 characters to the buffer, and will always create a NUL-terminated string value in the buffer. If the value of nMaxError is shorter than the actual error message text, the error message will be truncated, and there will be no indication that this has occurred, other than _tcslen(lpszError) == nMaxError - 1. If this is true, you should consider allocating a larger buffer and trying again.
Has anyone ever figured out why there is not a GetErrorMessageLength call to tell how large a buffer is needed, or better still, a method
virtual BOOL GetErrorMessage(CString & s, PUINT pnHelpContext = NULL)
so we don't have to create bogus fixed-size buffers and program as if we are writing K&R C code for the PDP-11?
The sad thing is that would actually have been easier to implement, in most of the handlers, than the code that is there! (Note that this could easily be done as an improvement, and the current version could create a temporary CString and call the new CString-based version, then copy the result to the LPTSTR).
(See also the documentation about why the set of flags documentation is poorly-written, misleading, and confusing)
The correct documentation of these is
CFile(); CFile(HANDLE hFile) CFile(LPCTSTR lpszFileName, UINT nOpenFlags) throw(...)
The Remarks section starts out by making a truly absurd statement:
Because this constructor does not throw an exception, it does not make sense to use TRY/CATCH logic. Use the Open member function, then test directly for exception conditions.
This is an out-and-out lie, and therefore must be corrected. Furthermore, it is contradicted by later documentation; the third paragraph in the Remarks section says
The constructor with two arguments creates a CFile object and opens the corresponding operating-system file with the given path. This constructor combines the functions of the first constructor and the Open member function. It throws an exception if there is an error while opening the file.
Unfortunately, it fails to mention that it is a CFileException *. The correct documentation would correct this by rewriting the first paragraph to say
The constructor without parameters and the constructor that takes a HANDLE do not throw exceptions.
And changing the wording in the third paragraph to say
The constructor with two arguments creates a CFile object and opens the corresponding operating-system file with the given path. This constructor combines the functions of the first constructor and the Open member function. It throws a CFileException * if there is an error while opening the file.
The example is erroneous, and the reference to the obsolete CATCH macro should be eliminated. The example uses the obsolete char data type, and does not properly handle the fact that CFile::Write can throw an exception.
The Remarks section, and this is verified by reading the code, says explicitly
The constructor with one argument creates a CFile object that corresponds to an existing operating-system file identified by hFile. No check is made on the access mode or file type. When the CFile object is destroyed, the operating-system file will not be closed. You must close the file yourself.
However, the comments in the existing example, which creates a CFile from a pre-existing handle, state
// We can call Close() explicitly, but the destructor would have
// also closed the file for us. Note that there's no need to
// call the CloseHandle() on the handle returned by the API because
// MFC will close it for us.
which is complete nonsense. It is too bad the person who wrote these comments had not actually read the documentation!
The correct example should be
HANDLE hFile = CreateFile(_T("C:\\MyFile.DAT"),
GENERIC_WRITE, FILE_SHARE_READ,
NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
if (hFile == INVALID_HANDLE_VALUE)
AfxMessageBox(_T("Couldn't create the file!"));
else
{
// Attach a CFile object to the handle we have.
CFile myFile(hFile);
static const TCHAR sz[] = _T("Hockey is best!");
// write string, without null-terminator
try {
myFile.Write(sz, lstrlen(sz) * sizeof(TCHAR));
}
catch(CFileException * e)
{
CString msg;
msg.Format(_T("Write failure: %d"), e->m_lOserror);
AfxMessageBox(msg);
}
// Note that if we call CFile::Close, we do not need to call ::CloseHandle
// on hFile. But we simply leave scope, and CFile::~CFile is called,
// it will not close the handle, and we will have to explicitly call
// ::CloseHandle at some later time.
myFile.Close();
}
In an intelligently-designed world, CFile::modeReadWrite would be equal to CFile::modeRead | CFile::modeWrite. For reasons that boggle the imagination, unlike the basic ::CreateFile call where GENERIC_READ | GENERIC_WRITE can (and must) be or-ed together to allow a file to be read/write, in MFC there is a totally separate value which is used to enable for reading and writing. Of course, the documentation makes no effort to point this obvious piece of poor design out to the reader. These errors are in the discussion of CFile::CFile.
Therefore, the documentation should read
Better still, additional information should be presented as follows
In the following table, the flags are grouped according to their compatibility
| Grouping | Selection | Options | Meaning |
| Creation | These flags are optional; none or all can be selected. | CFile::modeCreate | If present, the file is created; an existing file is truncated to zero length |
| CFile::modeNoTruncate | If present, will not truncate an existing file | ||
| CFile::modeNoInherit | Creates a non-inheritable file handle. If this is not specified, the file handle is inheritable | ||
| Access | Choose exactly one of these | CFile::modeRead | Opens file for reading only. May not be combined with CFile::modeWrite |
| CFile::modeWrite | Opens file for writing only. May not be combined with CFile::modeRead | ||
| CFile::modeReadWrite | Opens file for reading and writing | ||
| Share access | Choose exactly one of these | CFile::shareDenyNone | Allows subsequent read and write opens |
| CFile::shareDenyRead | Allows only read opens | ||
| CFile::shareDenyWrite | Allows only write opens | ||
| CFile::shareExclusive | If not specified, this is the default; no subsequent opens can be made | ||
| Data mode | Choose at most one of these | CFile::typeText | Sets text mode (used only for derived classes that use FILE *) |
| CFile::typeBinary | Sets binary mode (used only for derived classes that use FILE *) | ||
| Low-level options | Choose any of these, or all of them | CFile::osNoBuffer | Sets no file system caching: FILE_FLAG_NO_BUFFERING |
| CFile::osWriteThrough | Sets the write-through option: FILE_FLAG_WRITE_THROUGH | ||
| File access hint | Choose at most one of these | CFile::osRandomAccess | Suggests random access: FILE_FLAG_RANDOM_ACCESS |
| CFile::osSequentialScan | Suggests sequential access: FILE_FLAG_SEQUENTIAL_SCAN |
This erroneously states that it throws a CFileException if an error occurs. This is completely incorrect. It throws a CFileException * if an error occurs. Note that it is the responsibility of the frame that catches this exception to call the Delete method of the CException class to delete the exception object.
This class contains in its remarks a comment that has to rank among the most embarrassing Microsoft could put into any documentation:
When the user allocates their own buffer to accommodate OFN_ALLOWMULTISELECT, the buffer cannot be larger than 2048 or else everything gets corrupted (2048 is the maximum size).
It certainly causes one to wonder what insanity could have (a) created a situation in which a user allocated buffer length of the wrong size can "corrupt everything" (whatever that means!), (b) meant that such a fundamental bug is documented, instead of having fixed it immediately upon discovery, sending out a hotfix or service pack if necessary, and (c) originally resulted in choosing such a trivial size for such an important feature!
However, Giovanni Dicanio submits:
"The buffer can be larger than 2048, as several of us noted recently"
So apparently the bug has been fixed, but now the documentation is out of date! But is there now a limit? Or is there none? Is the bug fixed, or does it simply re-occur at a larger value?
This is another embarrassing example of poor documentation. It shows the declaration
static char BASED_CODE filter[] = "...";
which has numerous problems. First, it uses the qualifier BASED_CODE which is not hyperlinked to any documentation explaining what it is or what it does. The lack of the hyperlink saves trying to explain why, if you do go to the documentation, it says "Under Win32, this macro expands to nothing and is provided for backward compatibility". That is to say, this proves the documentation has not been reviewed by anyone competent since the 16-bit Windows version was written.
It allows the interpretation that the following declaration would be permitted:
void CMyView::OnDoSomething()
{
static char BASED_CODE filter[] = "...|...|...|";
CFileDialog dlg(TRUE, ..., filter);
....
if(dlg.DoModal() != IDOK)
return;(Note that due to terminal brain damage on the part of the Web designers, it was impossible to copy the text, and since I read these pages on a different machine that I work on, it was too painful to keep switching back and forth to try to copy the text out, so I just used "...." to indicate the stuff I didn't want to try to retype. Even stranger, when I right-click on the example, I get a capability of "saving it to Excel", which seems remarkably stupid, but if I try it, it tells me that a Web query could not return an answer. Has anyone actually tested these Web pages to see if they make sense?)
What is even worse is that this is not a Unicode-aware example; it should have been at the very least a non-static TCHAR, and have _T() around the quoted string, but in fact it is even worse than this: it suggests that it might even make sense to declare such a variable and initialize it to a literal string! This is so remarkably against modern practice that I wonder how anyone could have allowed an example this bad into the documentation. The correct approach is to make this a STRINGTABLE entry, and do
CString filter; filter.LoadString(IDS_EXCEL_FILTER);
because such terms as "Chart files" and "All Files" are clearly English, and to put literal English strings (or any language-of-your-choice) into a program today represents poor practice, since the string cannot be localized.
In the overview it says
In order for the parent window to be notified of find/replace requests, you must use the Windows
Hello? WHAT MESSAGE? The text should say
In order for the parent window to be notified of find/replace requests, you must use the Windows
Otherwise, I have to discover that this critical information is part of the Create description (I just looked in the header file to find it...)
The message will be sent only for Find Next and Cancel actions.
To detect actions other than Find Next and Cancel, you will need to subclass the CFindReplaceDialog dialog and create your own message handlers; because the wizards do not have knowledge of these conditions, you will need to hand-edit the message map to add these.
For example, to detect that the Find Whole Word option has changed, or the search text has changed, you might create a subclass CMyFindReplace You would add to the class definition
protected:
afx_msg void OnWholeWordClicked();
afx_msg void OnSearchStringChanged();
CButton c_WholeWord;
and add to the DoDataExchange function the line
DDX_Control(pDX, chx1, c_WholeWord);
and add to the message map
ON_BN_CLICKED(chx1, OnWholeWordClicked) ON_EN_CHANGE(edt1, OnSearchStringChanged)
(Do note that these entries do not end with a semicolon!). The symbols edt1 and chx1 are defined in the Platform SDK header file dlgs.h.
The handler for the Find Whole Word change event would then be
void CMyFindReplace::OnWholeWordClicked()
{
if(c_WholeWord.GetCheck() == BST_CHECKED)
{
...
}
else
{
...
}
}
The Remarks section is erroneous and confusing. It states
CFindReplaceDialog objects are constructed on the heap with the new operator. For more information on the construction of CFindReplaceDialog objects, see the CFindReplaceDialog overview. Use the Create member function to display the dialog box.
The correct document would say
CFindReplaceDialog objects must be constructed on the heap with the new operator. For more information on the construction of CFindReplaceDialog objects, see the CFindReplaceDialog overview. Use the Create member function to display the dialog box. Note that the PostNcDestroy handler of the CFindReplaceDialog will do a delete this, and if the object is not allocated on the heap, serious errors will result.
The description ends with
Within your OnFindReplace function, you interpret the intentions of the user and create the code for the find/replace operations.
Huh? How do I do this? Examine goat entrails? The correct documentation would say
Within your OnFindReplace function, you can use the FindNext and IsTerminating methods to determine which actions are being taken. The function is invoked for the Find Next and Cancel actions of the dialog. To handle other events, you would create a subclass of CFindReplaceDialog, as described in the CFindReplaceDialog overview.
The specification of the parameters is unclear. The correct specification would be
lpszFindWhat
Specifies the string for which to search. If this value is non-NULL, this string will be set as the search string when the Find/Replace dialog comes up. If this value is NULL, there will be no initial search string.
lpszReplaceWith
Specifies the string which will be used for the replacement. If this value is non-NULL, the string specified will be set as the replacement string when the Find/Replace dialog comes up. If this value is NULL (or the parameter is defaulted) there will be no initial replacement string.
It is not clear if this is complementary with IsTerminating or there is a condition under which the callback could take place where neither condition were true.
There is no discussion of how the user might implement this using a menu/toolbar command; for example, I used
static const UINT UWM_FIND_REPLACE = ::RegisterWindowMessage(FINDREPLACEMSG);
void CMyView::OnFindNext()
{
if(finder == NULL)
return; // no active find/replace dialog
PostMessage(UWM_FIND_REPLACE, 0, (LPARAM)finder);
}
The documentation of this is confusing. It assumes an unfortunate antecedent of 'the current'. It says
If this function returns nonzero, you should call the DestroyWindow member function of the current dialog box and set any dialog box pointer variable to NULL. Optionally, you can also store the find/replace text last entered and use it to initialize the next find/replace dialog box.
The "current" dialog box may mean something completely different to the user, such as the dialog box which is the application. A less confusing description would be
If this function returns a nonzero value, you should call the DestroyWindow member function of the current Find/Replace dialog and set any pointer variables which reference it to NULL. Optionally, you can also store the find/replace text last entered and use it to initialize the next find/replace dialog box. The CFindReplaceDialog::PostNcDestroy handler will call delete this to ensure the dialog box class instance for the Find/Replace dialog will be freed.
In addition, the example that is referenced under GetFindString has virtually nothing to do with the GetFindString example, and would be a perfect example for CFindReplaceDialog::IsTerminating, which is where it should appear!
Why is an example of termination used as an example of GetFindString? A better example for GetFindString would be an example that shows how to do whole-word detection including the boundary conditions at the beginning and ending of the buffer.
Note that it would be helpful if the example included how to load the strings and set them when a new dialog is created...it may be obvious to me, but not necessarily to someone else.
Also, it is not clear if IsTerminating and FindNext are complementary conditions, or there is a condition under which the callback could be called and neither of these predicates would be true?
These should be declared "obsolete" and maintained only for backward compatibility with older MFC code. The definitions should be
BOOL CFont::Create(....the long list of parameters here...); BOOL CFont::Create(const LOGFONT & lf)
There is no need to try to use the old Win16 API names any longer. Since this change is upward compatible (it is just adding to the existing class), it would simplify the documentation a lot.
These produce different results in VS .NET 2003 and VS 2005-and-beyond.
In VS .NET 2003, the font height was computed as
POINT pt; pt.y = ::GetDeviceCaps(hDC, LOGPIXELSY) * logFont.lfHeight; pt.y /= 720; // 72 points/inch, 10 decipoints/point
In VS 2005, the font height is computed as
POINT pt; // 72 points/inch, 10 decipoints/point pt.y = ::MulDiv(::GetDeviceCaps(hDC, LOGPIXELSY), logFont.lfHeight, 720);
Note that MulDiv rounds a fractional value up to the nearest integer, but the / operator truncates the value. So, for example, if the font height were specified as 80 units, that is, an 8point font, and the display had 96dpi, the computed height should be 96 * 80 / 720 = 10.67 pixels. But under the computation
pt.y = 96 * 11; // == 7680 pt.y /= 720; // == 10
but under MulDiv, the computation ::MulDiv(96, 80, 720) generates the result 11 because 10.67 is rounded to the nearest integer, 11.
The third member of the CHOOSECOLOR structure is erroneously defined and nonsensically explained.
It is defined as
HWND hInstance;
which of course makes no sense whatsoever because an HINSTANCE is not an HWND. Then it is further confused by the definition. The documentation in Flags is even more confusing.
hInstance
If the CC_ENABLETEMPLATEHANDLE flag is set in the Flags member, hInstanceis a handle to a memory object containing a dialog box template...
I have no idea what a "handle to a memory object" might be. I'm not even sure what a "memory object" is, because the way to reference a block of memory is typically by using a pointer. Perhaps it is meant to be an HGLOBAL to memory allocated by GlobalAlloc. Perhaps it is an HLOCAL to memory allocated by LocalAlloc. If either of these are true, then that is what should be stated. If both are true, that is what should be stated. I know it is confusing to actually tell the truth, but it would be useful here. For example
hInstance
If the CC_ENABLETEMPLATEHANDLE flag is set in the Flags member, hInstance holds a handle to a block of storage obtained via GlobalAlloc and which has been initialized to a dialog template. See Remarks. ...
Then, in the nonexistent Remarks section (which must be added), it would explain
When using CC_ENABLETEMPLATEHANDLE, the hInstance member must contain a handle to storage allocated by GlobalAlloc and initialized to hold a dialog template. While this is commonly obtained by FindResource/LoadResource, in that case it is more common to use the CC_ENABLETEMPLATE flag and set hInstance to the module instance required. If the template has been created by other means, it must either be created in storage allocated by GlobalAlloc or, once created, a suitably-sized allocation must be made using GlobalAlloc, then GlobalLock used to obtain a pointer to this storage and the template copied to this storage. The handle must eventually be unlocked via GlobalUnlock and the storage freed by GlobalFree.
Back in the description of hInstance, it goes on
...If the CC_ENABLETEMPLATE flag is set, hInstance is a handle to a module that contains a dialog box template named by the lpTemplateName member.
So it might be an HGLOBAL or an HLOCAL or an HINSTANCE. How could anyone have been stupid enough to declare it as an HWND? An HINSTANCE makes sense, or even the generic HANDLE type, but an HWND? Didn't anyone review these structures for sanity?
The documentation of Flags makes even less sense:
CC_ENABLETEMPLATEHANDLE
Indicates that the hInstance member identifies a data block that contains a preloaded dialog box template.
What is a "data block"? If I have one, how would I "identify" it? The usual way to indicate a reference to a block of memory is via a pointer. This is even less clear than "a handle to a memory object"! If it is supposed to be a handle of storage allocated by GlobalAlloc or LocalAlloc it should say so!
This will occasionally issue a completely erroneous message
Like most messages designed by amateurs, this is worse than content-free; it is out-and-out wrong. It is particularly annoying because this can occur on a machine with several hundred fonts installed, so it is obvious that the message is nonsense! The correct message would say
There are no fonts installed that match the selection criteria.
textual explanation of selection criteria here
Either change the selection criteria or install fonts that match the criteria
This is an example of a lazy programmer failing to analyze the reason for failure and using a simple, hardwired error message when an explanatory error message describing the exact reason would be the correct solution.
The printer DC provided via the CHOOSEFONT structure (or supplied as the third parameter to the CFontDialog constructor for CFontDialog) appears to have no effect on the font size returned. You would expect that if a DC for a 600dpi printer were supplied (which I do), then a 10point font should return an lfHeight of -83 (83/600 of an inch * 72ppi = 9.96 points), but instead it returns -19, which would be the correct value to return for a screen font. (The font selected was Arial).
In the overview of members, this is said to retrieve the name of the font. This is absurd. It retrieves a LOGFONT structure representing the selected font.
There appears to be a problem with the use of a printer DC to provide the reference DC for font selection. See CFontDialog::DoModal
The lpszStyle member is described as
Pointer to a buffer that contains style data. if the CF_USESTYLE flag is specified, ChooseFont uses the data in this buffer to initialize the font style combo box. When the user closes the dialog box, ChooseFont copies the string in the font style combo box into this buffer.
Well, this is not particularly useful information. What is "style information"? What format is it in? How large is the buffer? Could a short buffer have a buffer-overrun condition that would result in unsafe code? (There is no apparent maximum length specification of the size of this buffer).
The correct documentation should be at least
Pointer to a buffer that contains a style selection string. If the CF_USESTYLE flag is specifed, ChooseFont matches this string to a string in the font style combo box, and selects that style as the initial style displayed. The match is case-independent, but the match must otherwise be an exact string; a partial match will not be made (FindStringExact is apparently used to do the match). When the user closes the dialog box, ChooseFont copies the string in the font style combo box into this buffer. This means the parameter used cannot be a pointer to a literal string, which is stored in write-protected memory. <<What is missing here is a specification of the buffer size that should be provided>>.
Refers to the cmdID parameter as being selected from the CGID_MSHTML command group. There is no hyperlink to what this means. It does not mention that to use this API, to get those symbols, you must include the header file mshtmcid.h. There should be a hyperlink to the page named "MSHTML Command Identifiers".
To refer to this documentation as "hopelessly inadequate" is to overstate its quality.
There is no hyperlink to the type FXPT2DOT30 data type, which appears to be not documented at all. While it is clear that this is going to be represented as 11.111111111111111111111111111111 in a fixed-point representation, the representation is not explained; presumably the high-order bit is the sign bit, the next bit is either 0 or 1, and the remaining bits are the mantissa. But documentation would be useful.
This contains the obsolete keyword FAR. It is amazing that this was ever used to define a 32-bit-only concept, and that it would ever appear in a 32-bit structure description. FAR is dead, and should be deleted from every place it is used.
There are several forms of this method, and they are not explained; in the usual slovenly fashion, all parameters are explained as the union of all parameters in all overloaded methods, and no explanation is given. Frankly, I'm sick and tired of trying to reverse-engineer the methods from the union-of-every-possible-parameter descriptions, which I think is just a lazy documenter's way of avoiding doing the job right. Note that a throw statement is supposed to list the types of exceptions thrown, and if it doesn't, the notes should explain what exceptions are thrown! In spite of the nonsense the documentation people or implementors state, it is not possible to throw every conceivable exception ever defined. The number of exceptions is finite and enumerable.
I.
BOOL BitBlt( HDC hDestDC,
int xDest,
int yDest,
DWORD dwROP = SRCCOPY
) const throw( );
II.
BOOL BitBlt(HDC hDestDC,
const POINT& pointDest,
DWORD dwROP = SRCCOPY
) const throw( );
III.
BOOL BitBlt(HDC hDestDC,
const RECT& rectDest,
const POINT& pointSrc,
DWORD dwROP = SRCCOPY
) const throw( );
IV.
BOOL BitBlt(HDC hDestDC,
int xDest,
int yDest,
int nDestWidth,
int nDestHeight,
int xSrc,
int ySrc,
DWORD dwROP = SRCCOPY
) const throw( );
Notes:
In forms I. and II., the entire bitmap is transferred to the destination DC at the coordinates specified, starting at 0,0 in the source bitmap. In form III, the bits are transferred starting at the specified coordinates in the source, and only as many pixels as specified by the destination rectangle are transferred to the coordinates specified by the destination rectangle.
Form IV. provides full generality. Forms I..III are shortcuts for Form IV.
The exceptions that can be thrown are CMemoryException *...
This vaguely states that it loads an image file. It does not state that this image can be a BMP, JPG, GIF or PNG file. (You have to read a completely different part of the document to figure this part out). It also does not state that it can be a TIFF file.
This does not specify what type of resource is used to load the image; it vaguely says "contains the image to be loaded". Unfortunately, the only type of image it can load is a BITMAP resource, which should be explicitly stated. In fact, the correct documentation should say
Remarks
This uses the ::LoadImage API to load an IMAGE_BITMAP resource.
This talks about a "color mask" but contains no hyperlinks to suggest what this is or how it is used. A graphics wizard might find this obvious but it will not be obvious to the average reader who is trying to use a CImageList for the first time and may never have done any graphics programming. For example, it should link directly to the page that talks about "Drawing an image list" (ms-help://MS.VSCC.v90/MS.MSDNQTR.v90.en/dv_vclib/html/2f6063fb-1c28-45f8-a333-008c064db11c.htm in the VS2008 documentation).
This documentation presumes the reader has a clue as to what "IPF" means. Does it refer to the largely-dead "Itanium" processor? Who knows? There is no hyperlink on "IPF" to suggest what it means or explain it.
One of the apparent joys of this seems to be that the compiler has to cope with errors in the hardware. It is not clear if you want to produce something that can run on any version of whatever an IPF is what options should be selected.
The description /QIPF_noPIC says "Generates image with position independent code (IPF only)". Wouldn't the part that says "no" suggest that it means specifying this option creates an image with no position independent code? Duh?
This has always struck me as one of the more profoundly useless accretions in Visual Studio. I have never wanted to use it, but in VS2003+ it has become an essential part of the interface, even though there should be ways to browse what I want without ever seeing this piece of trash. For example, if I am editing dialogs, I should be able to have a dropdown that has the list of all the dialogs, and I just choose a class from that dropdown. I shouldn't need anything this elaborate.
But it is also delicate and temperamental. This is a feature I do not need in a programming environment. Suddenly, without warning, I get a presentation of the form
Why should I care about how many files it has to parse? There is apparently no attempt underway to actually parse them, and until this now-essential feature get these files parsed, I have lost the ability to add methods, handlers, etc. to my classes. I'm getting very good at doing this "by hand". I feel like I've fallen through a time warp to 1990 or so when we programmed in Microsoft C 3.0, used nmake, hand-built our MakeFile, etc. Have we learned nothing in nearly 20 years?
One thing that was clearly not learned was the value of documentation. Note that there is no "Help" button under these conditions that will take to your Happy Place where there is actual documentation explaining what this message means and what you can do to fix the problem. A search of the MSDN documentation reveals nothing; the Class View documentation naively expects that everything actually works all the time and therefore presumes that there is never a need to document the failure modes. So what's to do? Shutting down the project and deleting the .ncb file, followed by a full rebuild, does nothing. And, as it happens, I'm literally Out In the Middle Of Nowhere in Pontypŵl, Wales, UK, with no Internet service so no way of seeing if anyone else had this problem or how it was fixed. Right-click gives no useful information. I'd expect to see a menu that had the option "So parse them already, you are waiting perhaps for a sign from God?" to which I could issue some suitably scathing reply, such as "Oh, don't worry about me, I'll just sit here in the dark and type my program in by hand." (The fact is: I can. But if you teach, you discover that more and more students use Intellisense as a replacement for thinking, and simply can't recover if it doesn't work!)
I cannot make this go away. So I'm stuck. Key question: WHY IS THIS NOT DOCUMENTED? WHY IS THERE NO ACTIVE HELP LINK TO AN EXPLANATION WHEN IT OCCURS? ARE THERE ANY ADULTS PAYING ATTENTION TO THESE INTERFACES?
Without the Class View, I cannot add control handlers or menu handlers to my project using the tools.
Right before this failure, I got a message (which I now cannot reproduce) that said something about how an Add/Remove of an event was not possible because the code entity was read only. Well, I can imagine that if I had a read-only file and did not have the rights to modify that file, I should get such an error. But my file is read/write. I can read/write it from VS, from a secondary editor, and from NotePad if I have to. Therefore, the message is completely meaningless as an error report. But I get it anyway. On a file that mere minutes before I had successfully added a handler to. Now, the first thing we note about this message is that it is lying in it teeth. The file is clearly not write-protected, so any message to that effect is highly erroneous. What is going on? Well, obviously, I want to go to the documentation about this message. Right. And if you believe Microsoft has actually documented a message this important, then we really need to get together to help get my dead Nigerian uncle's funds from his bank...I'll give you 10% of the $30,000,000 he has stashed away. At which point I will retire and never have to write another line of code. You, on the other hand, will need to go into personal bankruptcy. That actually captures the Microsoft attitude pretty well.
The first solution is that every MessageBox that is produced by Visual Studio suggests a problem has a hyperlink (via its Help button, for example) to the MSDN page that explains what has gone wrong and what you have to do to fix it. Failing that, each such message shall have a code number such as RTE3002 that we can look up just like compiler error messages (alas, with probably as coherent an explanation. So it can be language-independent as compiler and linker messages are. Just look up the code. (Of course, having the Help button do it for us would be even better, but I'll settle for value of information over a fancy interface to it; I should be able to type RTE3002 into the search box and get the reference to it virtually instantaneously. (In my code, every MessageBox is unique in that there are absolutely, positively no two places that can issue the same text in a given MessageBox. Even if it issued by a common file-open handler, one of the parameters to that handler is information to be displayed that says who invoked the common handler code. I have no qualms about passing in __FILE__ and __LINE__ to an error message indicating a serious and unexpected failure. See my essay on creating useful text in message boxes.
Guiding principle: EVERY MessageBox that can be issued must uniquely identify the problem, give all essential details, and suggest a recovery technique or give a hyperlink to a local page that contains the information (do not link me to Microsoft Live! pages!!! I do not have internet access out here in South Wales (I barely have cell phone service!))
The documentation erroneously omits the NTAPI calling convention!
There are some serious errors in the essay on "Using the Clipboard". It is unfortunate these examples were not written by someone with a basic understanding of reality.
For example, in the specific example I looked at, the window function is declared as APIENTRY, when this violates the documented interface, which is CALLBACK. True, it just happens that in the current implementation APIENTRY and CALLBACK are both defined as synonyms for __stdcall, but that does not have to be true, and the use of APIENTRY for the CALLBACK function is erroneous.
The example is syntactically erroneous because it was written for Windows 1.0, before there was an ANSI C compiler. The quaint usage of not specifying the types in the parameter list, but after the parameter list, has been dead for 20 years! At least. Come on, isn't anyone actually reviewing these examples to see if they work at all?
The function uses a static variable inside the scope, hwndNextViewer, sets it in the WM_CREATE handler, and assumes it will be correct for all time. This is a deeply erroneous assumption; if there is ever a second instance of this window, this hack will fail utterly. This is per-window state and can only be valid for one window. It only works in this trivial example because there is only one instance of this window, but it does not generalize to MDI or any other faintly realistic scenario. To use a hack this easily broken in an example is unforgivable. It even encourages the idea that this technique might ever make sense. At the very least, the value should be stored as a property, and retrieved; the correct code would be
HWND hwndNextViewer; // note: this is NOT static!
#define PROP_HWND_VIEWER _T("HWND NextViewer") _T("guid-here")case WM_CREATE:
hwndNextViewer = SetClipboardViewer(hwnd);
SetProp(hwnd, PROP_HWND_VIEWER, (HANDLE)hwndNextViewer);
break;Then, when it is required, retrieve it:
case WM_DESTROY:
hwndNextViewer = (HWND)GetProp(hwnd, PROP_HWND_VIEWER);
ChangeClipboardChain(hwnd, hwndNextViewer);
PostQuitMessage(0);
break;
case WM_DRAWCLIPBOARD:
SetAutoView(hwnd);
hwndNextViewer = (HWND)GetProp(hwnd, PROP_HWND_VIEWER);
SendMessage(hwndNextViewer, uMsg, wParam, lParam);
break;Any solution which uses a static as illustrated is an example of amateur programming at its worst. I was writing code like this, using SetProp/GetProp, in 1990, in my first Windows program, so it is not Rocket Science to get it right!
As an alternative, the state can be stored in a struct pointed to by the GWLP_USERDATA field. This is considered Best Practice in modern standards if not programming in MFC, where the state is kept in the CWnd-derived class.
typedef struct {
HWND hwndNextViewer;
... other per-window state kept here
} UserData;
case WM_CREATE:
{
UserData * context = (UserData *)malloc(sizeof(UserData)); // or new UserData if using C++ without MFC
SetWindowLongPtr(hwnd, GWLP_USERDATA, context);
context->hwndNextViewer = SetClipboardViewer(hwnd);
break;
} case WM_DRAWCLIPBOARD:
{
UserData * context = (UserData *)GetWindowLongPtr(hwnd, GWLP_USERDATA);
SendMessage(context->hwndNextViewer, uMsg, wParam, lParam);
}It is also rather amateurish to put all the variables at the start of the block; good practice would place them local to the blocks in which they were used. That is, instead of a variable hwndNextViewer declared at the head of the function, the variables would be declared as needed:
case WM_DESTROY:
{
HWND hwndNextViewer = (HWND)GetProp(hwnd, PROP_HWND_VIEWER);
ChangeClipboardChange(hwnd, hwndNextViewer);
PostQuitMessage(0);
break;
}Better still, the windowsx.h macros should be used, so it should be written as
LRESULT CALLBACK MainWndProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
switch(uMsg)
{
HANDLE_MSG(WM_PAINT, main_OnPaint);
HANDLE_MSG(WM_SIZE, main_OnSize);
HANDLE_MSG(WM_CREATE, main_OnCreate);
HANDLE_MSG(WM_CHANGECBCHAIN, main_OnChangeCBChain);
HANDLE_MSG(WM_DESTROY, main_OnDestroy);
HANDLE_MSG(WM_INITMENUPOPUP, main_OnInitMenuPopup);
HANDLE_MSG(WM_COMMAND, main_OnCommand);
default:
return DefWindowProc(hwnd, uMsg, wParam, lParam);
}
return 0;
}The above represents Best Practice in writing a Windows app. Anything else is just amateur scribbling.
This appears to be broken under Vista. However, it is not clear why. For example, there is a qualification in the description of ClipCursor that "the calling process must have WINSTA_WRITEATTRIBUTES access to the window station", but there is no hyperlink to the concepts of "Window station" or "WINSTA_WRITEATTRIBUTES". Any attempt to figure out how to use GetSecurityInfo wanders off into obscure security features that make it hard to even attempt to write code to see if this attribute is set, or figure out how to set it if it is not set.
This section fails to contain any explanation of the form
"For multiple-selection list boxes, this will not work. Instead, using GetSelItems to get an array of currently-selected items"
nor does the See Also section contain a reference to GetSelItems.
Does not mention that to get the current selection of a single-selection listbox, GetCurSel should be used, nor does it contain a hyperlink to GetCurSel in the Remarks section nor in the See Also section.
This does not contain a hyperlink to GetSelCount nor does GetSelCount appear in the See Also section.
Also, the examples that include declarations like
extern CListBox * pmyListBox;
are confusing and misleading to most readers. It is rare that a pointer to a listbox is involved; more likely, a CListBox control variable appears in the class definition. The examples should all be of the form
// The declaration in the class, created by the Add Variable tool, would be CListBox myListBox; // This code would appear in a handler for some event in the dialog, property page, or form view int nCount = myListBox.GetSelCount(); CArray<int> aryListBoxSel; aryListBoxSel.SetSize(nCount); myListBox.GetSelItems(nCount, aryListBoxSel.GetData()); // Dump the selection array #ifdef _DEBUG afxDump << aryListBoxSel; #endif
This would save the insane code I've seen where somebody declares a global variable, assigns to it the address of the control variable, and uses it in the style shown in all too many of these examples. Note there is no need to specify the second template parameter to CArray
Note that the rgIndex parameter is specified as "a long pointer". Whatever that is. Who knows what a "long pointer" is? Quickly, everyone! (Those who wrote programs for Win16 are not eligible to answer). Goes along with the stupidity of including "FAR" in declarations, header files, and documentation. This concept has been obsolete in Win32 since its creation, so why does the documentation still suggest that such a concept exists?
Does anyone know what the "rg" prefix means in Hungarian Notation? And shouldn't this simply be called "nCount" which would make more sense?
The code example shows
int n = pMyListBox->InitStorage(256, 10);
ASSERT (n != LB_ERRSPACE);
CString str;
for(int i = 0; i < 256; i++)
{
str.Format(_T("item string %d"));
pMyListBox->AddString(str);
}
The problem with this is that the second parameter is the number of bytes to allocate for strings. "item string 256" takes 16 bytes (including the terminal NUL) in 8-bit apps and 32 bytes in Unicode. So why was "10" chosen as the example string length?
(Thanks to Robert Zimmerman for pointing this out)
The question also arises: what about dealing with Item Data? If I'm going to have item data, and want to preallocate 256 items, should I add 256 * sizeof(MyItemDataStruct)? If it is a regular ListBox, or owner-draw with LBS_HASSTRINGS, should I allocate 256 * (sizeof(MyItemDataStruct) + STRING_ALLOWANCE) where STRING_ALLOWANCE is the number of bytes I plan to allocate for the strings? Inquiring Minds Want To Know!
This states that it cannot be used to set or remove a selection in a multiple-selection list box, but then fails to give a hyperlink to CListBox::SetSel, nor does CListBox::SetSel appear in the See Also section.
Does not contain a hyperlink that says "To set the selection for a single-selection ListBox, use SetCurSel", nor does it include SetCurSel in the See Also section.
The function CListCtrl::SortItems takes a user-defined parameter of type DWORD_PTR but the callback function takes this as a type LPARAM. How is this? Yes, we know that the Deep Truth is that both DWORD_PTR and LPARAM are types that are either 32-bit or 64-bit, but in fact a DWORD_PTR is unsigned and an LPARAM is signed. How is it that someone could write an interface spec that used two different types to mean the same thing? Which is it?
When given an invalid handle this will call RaiseException to throw a Structured Exception. The exception code is 9. This is undocumented, and also incompatible with C++.
Like most MFC locking primitives, there is a fundamental simple principle to guide the usage of this type:
NEVER, UNDER ANY CIRCUMSTANCES IMAGINABLE, EVER, USE A SYNCHRONIZATION PRIMITIVE DEFINED BY MFC!!!!!!!!!
It's that simple. But here's the proof:
The correction to the documentation is to label these classes as "broken, and broken by design, not to be used".
In the superclass code, the call to CSyncObject::Lock is as shown below
BOOL CSyncObject::Lock(DWORD dwTimeout)
{
DWORD dwRet = ::WaitForSingleObject(m_hObject, dwTimeout);
if (dwRet == WAIT_OBJECT_0 || dwRet == WAIT_ABANDONED)
return TRUE;
else
return FALSE;
}
One can only look upon the author of this code with pity and scorn. Note what this code says: "If someone is in the middle of modifying a protected area of data, and crashes before completing the modifications, you may now proceed to use that data as if it has been correctly modified, even though you know it is probably corrupted and will cause you to crash unpleasantly, or corrupt the data, or lose information, In addition, every thread that was blocked on the mutex will receive the WAIT_ABANDONED notification, so now all these threads think they have successfully acquired the mutex, and they all happily proceed to try to use the data, concurrently.
But hey, do you want us to do the job right? That would take work, because we'd have to return an actual error code, and we only want to return a BOOL, and besides, it doesn't matter that you now have several threads concurrently accessing your allegedly protected data."
Such code is beneath contempt. It is a prime example of how to guarantee catastrophic failure of your program.
But it's worse than you can imagine. A mutex, like a critical section, has "recursive acquisition semantics", that is, if acquired more than once by the same thread, the acquisition merely increases a reference count, and there must be as many releases as acquisitions. The proof is that you can write
HANDLE lock = ::CreateMutex(NULL, FALSE, NULL); ::WaitForSingleObject(lock, INFINITE); ::WaitForSingleObject(lock, INFINITE); ::ReleaseMutex(lock); ::ReleaseMutex(lock); ::CloseHandle(lock);
(You may ask why you would lock twice in a row; the above code shows a logical presentation of what can be a complicated execution sequence spanning many functions). But the above code works. Now let's look at CMutex in the same context:
CMutex crit; CSingleLock lock(&crit); lock.Lock(); lock.Lock(); lock.Unlock(); lock.Unlock();
You would think this would work, but it does not and cannot be made to work. Let's look at the lock and unlock code in MFC:
BOOL CSingleLock::Lock(DWORD dwTimeOut /* = INFINITE */)
{
ASSERT(m_pObject != NULL || m_hObject != NULL);
ASSERT(!m_bAcquired);
m_bAcquired = m_pObject->Lock(dwTimeOut);
return m_bAcquired;
}
BOOL CSingleLock::Unlock()
{ ASSERT(m_pObject != NULL);
if (m_bAcquired)
m_bAcquired = !m_pObject->Unlock();
// successfully unlocking means it isn't acquired
return !m_bAcquired;
}
How many fundamental design errors can you spot in this code? Let's see, we can't pass a pointer to it to another thread because the m_bAcquired is not using any form of synchronized access. It assumes that recursive acquisition can never exist, because it assumes on a Lock call that m_bAcquired is FALSE. It is set if the lock is acquired. If it is not set, the Unlock code cannot be executed.
How, exactly, this could ever possibly make sense for a mutex, semaphore, CRITICAL_SECTION or event escapes me. It cannot make sense. For example, and I discovered this years ago, if the Mutex has a name and is shared between processes, each process will have its own CSingleLock object and the correlation of the m_bAcquired state and the actual state is impossible to maintain.
Had the m_bAcquired variable been eliminated, this code could be made to work, but as it stands, this code is complete crap, and must never be used (unless, of course, you have no interest in the correctness of your code). Combine this with the failure to properly handle WAIT_ABANDONED and you have a class that is guaranteed to make your code incorrect.
The documentation says
When used with .MODEL, starts a code segment (with segment name CODE)
.CODE
Unfortunately, there is nothing in the description of .MODEL that suggests in any way whatsoever that .CODE has any meaning. I can only infer that this documentation is gibberish.
The same error appears for .CONST and .DATA.
This is a superclass which has a very serious bug. It is not transparent to the ::GetLastError value. Due to erroneous design in the OLE functions, some low-level OLE handler (called eventually by a Release call from the destructor) incorrectly sets the ::GetLastError value to 0 (S_OK). Unfortunately, this means that if you try to write intelligently-designed code, such as
if(some_error_condition)
{ /* operation failed */
DWORD err = ::GetLastError();
... process error
::SetLastError(err);
return FALSE;
} /* operation failed */
and you have any OLE automation object in the scope of your function, the error value is destroyed, so the caller, when it gets the FALSE return and calls ::GetLastError to see why, gets S_OK (0) instead of the error value you set! In my case, it was the CPresentation type involved with a PowerPoint Automation Interface call.
The documentation says
When used with .MODEL, starts a constant data segment (with segment name CONST)
.CONST
Unfortunately, there is nothing in the description of .MODEL that suggests in any way whatsoever that .CONST has any meaning. I can only infer that this documentation is gibberish.
The same error appears for .CODE and .DATA.
(Well, this is actually a Excel documentation bug, but why not add it here...)
The Excel documentation says
Remark
If the angle is in degrees, multiply it by PI()/180 or use the COS function to convert it to radians.
The correct documentation is
Remark
If the angle is in degrees, multiply it by PI()/180 or use the RADIANS function to convert it to radians.
This is an exceptionally-poorly-written example. It does not, for example, show that there is a variable called my_unique_prefix1, nor does it show how you might use this code realistically in some context! The example is contrived to the point of uselessness. It uses a previously-undefined term, "compiland", to describe its use. No example is shown by which I might do something like
#define DoSomething(x) \
int FUNC(MyVariable); \
MyVariablen = ...; \
CallSomeFunction(MyVariablen);
and therefore illustrate how to get references to the actual declaration of MyVariablen. Instead, it presumes you know that the current value of __COUNTER__ was 0, and there is no reason to expect this!!!! A useful example should be provided.
The documentation also does not state that this value is defined only if _MSC_VER >= 1300.
The documentation erroneously claims the function is BOOL and returns TRUE if the path is a directory, FALSE otherwise. A correspondent [thanks to Mikel Luri Fernandez] points out that this documentation is incorrect; it returns a non-zero value if the path is a directory, in particular, it returns the value 16, which is the value for FILE_ATTRIBUTE_DIRECTORY.
The documentation of this is nonsensical. First, who is Dinkumware and why should we care? Second, what is "reports" relative to a symbol? It might say "is defined as", but the verb "reports" is meaningless. Third, what is the format of the value? Inquiring Minds Want To Know! Note that as soon as this takes on more than one value, the complete history of values shall be displayed, and preferentially with an explanation (e.g., "TR1", with a hyperlink to what TR1 actually means).
It is defined in yvals.h, which is not even part of the C++ Standard Library.
Empirically, I have discovered the following
| VS Version | _CPPLIB_VER |
| VS6 | (not defined) |
| VS2003 | 313 |
| VS2005 | 405 |
| VS2008 | 505 |
Note: I happen to know that Dinkumware created the Standard Template Library (STL), now known as the Standard C++ Library, and most of it was written by P.J. Plauger. But how is anyone else going to know that? Only C++/STL geeks would know that information!
The documentation says
A bit mask specifying the processor mode (numeric equate).
I'm sure this actually has real values, but apparently they are top-secret, and cannot be determined unless you know someone at Microsoft.
Documentation like this is an insult to intelligent readers.
It is not clear why this simple and obvious table does not appear in the documentation: And it would be helpful if the "clever" encoding were actually explained; I've made a few guesses at the end as to what the bits might mean.
Some days, Microsoft is their own worst enemy when the issue of "open source" arises! Slovenly excuses for documentation are a major factor that make them look really bad!
| Directive | @CPU value | ||
| Hex | Binary | Decimal | |
| .8086 | 0101h | 0001 0000 0001 | 257 |
| .8087 | 0101h | 0001 0000 0001 | 257 |
| .NO87 | 0001h | 0000 0000 0001 | 1 |
| .186 | 0103h | 0001 0000 0011 | 259 |
| .286 | 0507h | 0101 0000 0111 | 1287 |
| .286P | 0587h | 0101 1000 0111 | 1415 |
| .287 | 0501h | 0101 0000 0001 | 1281 |
| .386 | 0D0Fh | 1101 0000 1111 | 3343 |
| .386P | 0D8Fh | 1101 1000 1111 | 3471 |
| .387 | 0D01h | 1101 0000 0001 | 3329 |
| .486 | 0D1Fh | 1101 0001 1111 | 3359 |
| .486P | 0D9Fh | 1101 1001 1111 | 3487 |
| .586 | 0D3Fh | 1101 0011 1111 | 3391 |
| .586P | 0DBFh | 1101 1011 1111 | 3519 |
| .686 | 0D5Fh | 1101 0101 1111 | 3423 |
| .686P | 0DDFh | 1101 1101 1111 | 3551 |
| .K3D | 0D01h | 1101 0000 0001 | 3329 |
| The following bits are inferred by inspection of the above patterns | |||
| protected mode instructions | 0080h | 0000 1000 0000 | 128 |
| floating point | 0100h | 0001 0000 0000 | 256 |
| ? | 0400h | 0100 0000 0000 | 1024 |
| has virtual hardware mode | 0800h | 1000 0000 0000 | 2048 |
The API documentation is a bit confusing, and also has a tendency to lie.
For example, it says
If the function fails, the return value is NULL. The function will return NULL for a DEVMODE structure other than the current DEVMODE....To get extended error information, call GetLastError.
Nobody can figure out what a "current DEVMODE" is, and the entire sentence about returning NULL in such a case appears to be nonsensical. It suggests that either the sentence should be removed, or a substantial amount of explanatory material should be added.
It has been reported several times that CreateDC will return NULL but GetLastError returns ERROR_SUCCESS. This appears to be a function of the printer driver, and suggests that there are bugs in some drivers. Therefore, the advice that you can get additional information from GetLastError could be a lie, but no discussion of the conditions under which this can occur is given.
CreateDialog and CreateDialogIndirect are specified as
HWND CreateDialog(HINSTANCE hInstance, LPCTSTR lpTemplate, HWND hWndParent, DLGPROC lpDialogFunc); HWND CreateDialogIndirect(HINSTANCE hInstance, LPCDLGTEMPLATE lpTemplate, HWND hWndParent, DLGPROC lpDialogFunc); HWND CreateDialogParam(HINSTANCE hInstance, LPCTSTR lpTemplate, HWND hWNdParent, DLGPROC lpDialogFunc, LPARAM lParam); HWND CreateDialogIndirectParam(HINSTANCE hInstance, LPCDLGTEMPLATE lpTemplate, HWND hWndParent, DLGPROC lpDialogFunc, LPARAM lParam);
All of these erroneously claim that the hWndParent is used as the parent window for the dialog. However, this is erroneous; experiment demonstrates that the parent is always the main application window.
The documentation is confusing and misleading. For example, it states that
ppvBits
Pointer to a variable that receives a pointer to the DIB bit values
[This seems pretty obvious. But then in the description of hSection we find bizarre and confusing information]
hSection
...
If hSection is NULL, the system allocates memory for the DIB.
[so far, this seems clear]
In this case, the CreateDIBSection ignores the dwOffset parameter.
[so far, this seems clear]
An application cannot later obtain a handle to this memory.
[This is out of left field. Why in the world would an application want a handle to this memory? What possible sense could this make? It isn't even clear what purpose a handle would serve, given we have explicitly said we are not supplying a handle, so why are we bothered with information that has no value, and only serves to be confusing?]
The dshSection member of the DIBSECTION structure filled in by calling the GetObject function will be NULL.
[And so what? Who cares? If I specify it as NULL when I create the DIB section, I should expect to see that it is, to no one's surprise, NULL if I ask for it later! So why throw in nonsense about memory handles that has nothing to do with anything imaginable?]
When a parameter may be NULL, an API description should say so at the point where the parameter is specified, not buried in the remarks. Thus, this should say
hdcRef
[in] Handle to a reference device for the enhanced metafile. May be NULL, see Remarks
There is no discussion of how to close an enhanced metafile! There is no hyperlink to ::CloseEnhMetaFile.
This does not have a hyperlink to LOGFONT for every instance of LOGFONT. There is an erroneous myth about Web page design that says you should not hyperlink a term more than once; this rule was created by idiots. EVERY instance of a term that is hyperlinked should be hyperlinked, especially if the nearest adjacent use of the term is not visible on the same screen. A rule like this was designed by people who never actually have to use Web pages, but once read some book about Web page design and therefore think they have some say in the design, and propagate this piece of idiocy from generation to generation of document designers. In addition, there is no instance of LOGFONT in the See Also section! Every field name should have a hyperlink to the exact paragraph in the LOGFONT description! Also, EnumFontFamilies is not hyperlinked, but does at least appear in the See Also section.
What is not discussed here is the matching algorithm. In an intelligently-designed world, the facename would dominate; that is, whatever else was specified would be thought of as the "decorations" of the face name. But in Windows, a deeply-flawed model is used; the facename is the least important parameter to the name. So if you want a Wingdings font that is the same size as the standard font you are using, you can't just ::GetObject/CFont::GetLogFont, change the face name, and create a font from that information. Instead, you must magically infer that you need to change several other parameters to get it to work right.
lf.lfPitchAndFamily = FF_DECORATIVE; lf.lfCharSet = SYMBOL_CHARSET;
Yet these should have nothing to do with the problem! I already said what font I want! Everything else is irrelevant in the font choice!
See also CFont::CreateFont.
There is a discussion about how there are "byte pipes" and "message pipes" but there is not, and never has been, any description of what a "message pipe" is, or how it is different from a "byte pipe".
In a separate article, titled "Named PipeType, Read, and Wait Modes", there is a semi-coherent explanation. It might be interpretable as follows, but this is my deduced summary of what it says:
A pipe can be opened in "byte mode" or "message mode". In "byte mode", a pipe is simply a stream of bytes; in "message mode", a pipe is a stream of "messages", where a "message" is a single ReadFile or WriteFile operation. The behavior is as follows:
Synchronous pipes: WriteFile will write a sequence of bytes. For a byte-mode pipe, this represents an arbitrary number of bytes, nothing more. In a message-mode pipe, this represents a single "message".
Asynchronous pipes: In byte mode, WriteFile will write as many bytes as it can, and return the number of bytes actually written. The number of bytes it can write is limited by the available internal buffer space; this is more likely to have a major impact when the client and server are on separate machines. When writing to a named pipe, a WriteFile operation can return TRUE (success) even if it did not write all the bytes, and it will return in the bytesWritten parameter the number of bytes actually written. In message mode, there is no attempt to write a partial buffer.
Synchronous pipes: In byte mode, ReadFile will read the number of bytes available, up to the maximum buffer size. "Message" boundaries based on how the sender issued the WriteFile have no meaning and are ignored, whether the sender wrote the bytes in byte mode or message mode. In message mode, ReadFile will read an entire message (up to the maximum buffer length) if the bytes were written in message mode, but the number of bytes read will not exceed the number written by a message-mode WriteFile. If the buffer size is too small, the bytes are read, but ReadFile returns FALSE and GetLastError returns ERROR_MORE_DATA. subsequent ReadFile operations will read remaining pieces of the message, but not exceeding an individual message.
The documentation is missing several key pieces of information.
lpDisplayName
lpBinaryPathName
lpLoadOrderGroup
lpdwTagId
If this value is non-NULL, the tag will be recomputed based on the load order vector and an updated tag will be returned in the DWORD which is referenced. If it is NULL, description needed.
At the end of the description, there is a a poorly-written table which suffers from many omissions. It does not state the type of a key, and it does not explain the keys properly.
It also fails to note that the description of the DisplayName is a REG_SZ, but can still contain expandable values if it starts with an @ (the natural inclination would be to think this must be a REG_EXPAND_SZ value to allow that). It also fails to mention the RequiredPrivileges key, which must be a REG_MULTI_SZ, and there is would have to be a link or an enumeration explaining what the values could be. Thus, the table should be of the form shown below. Note that the appearances of ChangeServiceConfig2 must be active hyperlinks.
| Key | Type | Description |
| DependOnGroup | REG_MULTI_SZ | Load-ordering groups on which this service depends, as specified by lpDependencies. |
| DependOnService | REG_MULTI_SZ | Services on which this service depends as specified by lpDependencies |
| Description | REG_SZ | The description as specified by ChangeServiceConfig2. This may be of the form "@pathname,-stringid". |
| DisplayName | REG_SZ | The display name as specified by lpDisplayName. This may be of the form "@pathname,-stringid". |
| ErrorControl | REG_DWORD | The error control value as specified by dwErrorControl. |
| FailureActions | REG_BINARY | Failure actions specified by ChangeServiceConfig2 |
| Group | REG_SZ | Load ordering group specified by lpLoadOrderGroup. Note that setting this value can override the setting of the DependOnService value. |
| ImagePath | REG_EXPAND_SZ | A string specifying the location of the executable image, as specified by lpBinaryPathName |
| ObjectName | REG_SZ | Account name specified by lpServiceStartName |
| RequiredPrivileges | REG_MULTI_SZ | ? who knows ? |
| ServiceSidType | REG_DWORD | ? who knows ? |
| Start | REG_DWORD | When to start the service, as specified by dwStartType. |
| Tag | REG_DWORD | Tag identifier which was returned in lpdwTagId. If lpdwTagId is not specified, ? who knows ? |
| Type | REG_DWORD | Service type specified by dwServiceType |
It is not clear why the Group attribute says it "can" override the setting; either it does or it doesn't. If there are conditions that are implied by "can", then they must be explained. There is no explanation.
In examining a FailureActions element, the following structure was found:
FF FF FF FF DWORD dwResetPeriod 00 00 00 00 LPTSTR lpRebootMessage 00 00 00 00 LPTSTR lpCommand 03 00 00 00 DWORD cActions 14 00 00 00 ? 00 00 00 00 SC_ACTION_TYPE 00 00 00 00 DWORD delay 00 00 00 00 SC_ACTION_TYPE 00 00 00 00 DWORD delay 00 00 00 00 SC_ACTION_TYPE 00 00 00 00 DWORD delay
It is not clear what the 0x14 value is. It is too short to be the length of the rest of the structure (which is 0x18). So it is not clear how to interpret these values.
Does not state that this must not be called in DllMain or deadlock will result.
The parameter is stated to be "reserved", but correct documentation would state "The parameter is reserved, and must be specified as NULL".
It does not mention that the lpWindowName parameter can be NULL.
If the window style is given as WS_POPUP, the hwndParent parameter is ignored, and the parent returned by ::GetParent is not the parent of the window, but the top-level window of the application. This can be verified by using Spy++ to inspect the parent link, which is indeed the handle to the top-level window, not the handle that was passed to ::CreateWindow.
If the parent is HWND_MESSAGE, the documentation is not quite correct. See the discussion of HWND_MESSAGE.
It does not mention that the lpWindowName parameter can be NULL.
If the window style is given as WS_POPUP, the hwndParent parameter is ignored, and the parent returned by ::GetParent is not the parent of the window, but the top-level window of the application. This can be verified by using Spy++ to inspect the parent link, which is indeed the handle to the top-level window, not the handle that was passed to ::CreateWindowEx.
If the parent is HWND_MESSAGE, the documentation is not quite correct. See the discussion of HWND_MESSAGE.
Compared to the other articles, Writing a Service Program's main function, Writing a ServiceMain function, Writing a Control Handler Function, this is actually a well-written example. It is Unicode-aware, for example.
However, it is not clear why the first SERVICE_TABLE_ENTRY name is an empty string. It has to be pointed out that this only applies to a SERVICE_WIN32_OWN_PROCESS.
However, it does use the dangerous and obsolete wsprintf, instead of something safe. In VS2005, it should use _stprintf_s which is both safe and Unicode-aware.
It shows multithreading, but it hardwires the value 3 instead of doing something sensible like doing
#define MAX_THREADS 3
or
const int MAX_THREADS = 3;
which would make things much clearer.
It should not be calling CreateThread, but should call _beginthread, _beginthreadex or AfxBeginThread to start the threads, otherwise the threads may not correctly work in the environment.
It is not clear why the WaitForSingleObject is timing out at 1000ms, or why there is something that says "Main loop for service" at all. It would make more sense to have
WaitForSingleObject(hStopEvent, INFINITE);
since there is essentially nothing to do until the event is signaled. The example implies that there is actually something useful that might be done here, and there rarely is.
The correct code for the loop is
for(t=1; ; t++)
since the TRUE is redundant in the code shown.
It should be pointed out that the purpose of the loop is to allow a new checkpoint value to be computed each time the timeout happens, and, for example, it has nothing to do with the number of threads.
The test for WAIT_ABANDONED is pointless because there is no mutex involved in the wait, and WAIT_ABANDONED is therefore impossible. There should be an explanation that the reason there is a timeout in the WaitForMultipleObjects is so the SERVICE_STOP_PENDING can be executed once a second.
The use of the same string TEXT("CloseHandle") for three completely different error conditions is poor style; it should say something like
if(!CloseHandle(hStopEvent))
ErrorStopService(TEXT("CloseHandle(hStopEvent)"));
There is a perfectly fine switch statement to discriminate the control events. Having a separate if-test that then re-decodes the dwCtrlCode a second time is silly. If the excuse is that "there are several lines of code there", then the correct solution is to call a subroutine to do all that. But since the switch statement does exactly the same thing in both cases, why not just combine the cases as well?
case SERVICE_CONTROL_SHUTDOWN:
case SERVICE_CONTROL_STOP:
dwState = SERVICE_STOP_PENDING;
DoShutdown();
break;
The use of the SetTheServiceStatus function to prepare the structure and call SetServiceStatus represents best practice and therefore should be moved to the other really bad examples already mentioned above.
However, this does not allow for service-specifc errors. It should have an additional parameter, dwServiceExitCode. dwServiceExitCode is nonzero, the dwWin32ExitCode should be set to ERROR_SERVICE_SPECIFIC_ERROR, e.g.,
ss.dwServiceSpecificExitCode = dwServiceExitCode; ss.dwWin32ExitCode = (dwServiceExitCode != 0 ? ERROR_SERVICE_SPECIFIC_ERROR : dwWin32ExitCode
It is extremely unclear why programmers feel an overwhelming compulsion to zero out buffers they are about to completely overwrite; the ZeroMemory calls should be eliminated, because the wsprintf call completely overwrites the significant errors. And, of course, the obsolete and dangerous wsprintf should be replaced by _stprintf_s.
The complex for-loop that is used to shut down the threads should not be duplicated here! A single function that implements that logic should be called from both sites.
This code is not an example of best practice, but it is orders of magnitude better than the incredibly poor examples used for other threading.
The _CrtMemCheckpoint function does not work as documented. I have a complete essay on this topic.
The documentation of this function is erroneous and misleading. It suggests that the size parameter of the hook function specifies the size of the memory block, in bytes, but for _HOOK_FREE, this value is always 0! It is not clear why the function neglects to send this critical information across the boundary. In addition, the filename and lineNumber parameters say "if available" but do not state what is meant by "available". "Unavailable" means they will be NULL and 0.
If there were any attempt at doing respectable documentation, it would say
| allocType | _HOOK_ALLOC | _HOOK_FREE | _HOOK_REALLOC |
| userData | NULL | Pointer to user block | NULL |
| size | Size of allocation | 0 | Size of reallocation |
| blockType | Block Type | Block Type | Block Type |
| requestNumber | Request number | 0 | Request number |
| filename | File name, or NULL | NULL | File name, or NULL |
| lineNumber | Line number, or 0 | 0 | Line number, or 0 |
This class cannot be used with CSingleLock because the destructor of CSingleLock will call ::ReleaseSemaphore, which would result in erroneous behavior. Furthermore, it cannot be used across processes with a named semaphore, because an attempt to acquire it sequentially in one process will result in erroneous behavior. In addition, CSingleLock::Unlock will not allow a semaphore to be released more than once, which means it cannot be used in a receiver loop that needs to release the semaphore many times; the CSingleLock::Unlock goes out of its way to get this wrong.
This erroneously states "ReleaseBuffer will perform a strlen on the buffer to determine its length". This is erroneous. The correct documentation is "ReleaseBuffer will perform a tcslen on the buffer to determine its length". A CSimpleStringT could be a Unicode string.
The specification of the parameter nMinBufferLength makes no sense. It says
The minimum size of the character buffer in characters. This value does not include space for a null terminator.
I have no idea what this means. It might mean
The minimum size of the character buffer, expressed as a character count. The buffer will hold this many characters, and sufficient space will be allocated to allow for nMinBufferLength characters followed by a null character.
but if that's what it means, it should say so. It also says
If there is insufficient memory to satisfy the GetBuffer request, it throws an exception.
Really? What exception? Perhaps the correct documentation would have said
If there is insufficient memory to satisfy the GetBuffer request, it throws a CMemoryException *.
From reader Jim Beveridge:
This additional information should be added to the documentation: Do not ever call ReleaseBuffer without first calling GetBuffer because ReleaseBuffer ignores the current reference count and so does not perform “copy on write” semantics. If you need to truncate a CString, use Truncate().
The discussion of this class is largely nonsensical, except in the places where it is grossly incorrect.
First, and most important, it fails to ever mention that CSingleLock does not allow recursive acquisition of any synchronization object, meaning you cannot correctly acquire a CMutex or CCriticalSection recursively, cannot use CSemaphore, CMutex or CEvent across process boundaries, cannot use a CSemaphore or CEvent in a loop, and generally can't do anything right. It contains no link to CSingleLock::Lock. The description
To use a CSingleLock object, call its construct inside a member function of the controlled resource's class. Then call IsLocked member function to determine if the resource is available; if it is, continue with the remainder of the member function. If the resource is unavailable, either wait for a specified amount of time for the resource to be released, or return failure.
It does not mention that the destructor CSingleLock::~CSingleLock will call the CSingleLock::Unlock method, making this completely unusable for a CSemaphore class! In fact. the destructor is undocumented.
What part of "lock" did the twit who wrote this code miss? Why is there no reference to CSingleLock::Lock? How is it that CSingleLock::IsLocked can return TRUE if there is no attempt made to lock it? How is it that the suggested approach is to poll instead of block the thread instead of waiting for the lock? What child wrote this, and where was the adult supervision? I have never seen a poorer explanation of synchronization than this.
This does not mention that the initial acquisition might fail and the constructor will block, which means it would be a very poor idea to specify bInitialLock=TRUE in any real object. It does not mention that the destructor will call CSingleLock::Unlock, which means it is unusable with CSemaphore. It replicates the poor example given in all other methods, where CSingleLock::Lock is called but the value is not tested, but IsLocked is called as if there could be no race condition. In this trivial example that is true, but there is no discussion about the fact that a CSingleLock object cannot be shared with other threads.
This method is completely undocumented, even though it can cause massive malfunction. For example, an attempt to apply CSingleLock to a CSemaphore will result in an erroneous program.
There are so many things wrong with this documentation that it is unusable in its current form. For example, it specifies a timeout, but in the Remarks section it does not mention that if it is applied to a CCriticalSection class that the timeout must always be INFINITE? Or that for CCriticalSection, no mater what timeout is used, it will be treated as INFINITE after the ASSERT failure. At no point, does it mention that recursive acquisition is impossible, or that it will not work if the CSingleLock object is shared across threads, or if you are locking across processes. It does not mention this cannot be made to work correctly. It does not mention that for CMutex, if the error is WAIT_ABANDONED, it treats it as a successful return. The example is nonsense. If the CSingleLock fails, it is important to know why, but this information is not made available; testing IsLocked might mean that the object is locked, or it might mean that object has been locked in a second thread. Since there is no documentation that the CSingleLock object cannot be shared across threads, it is therefore possible to create programs that cannot correctly work, but for some brief period of time will give the illusion of working.
If a CSemaphore is locked using CSingleLock::Lock, when the variable goes out of scope, the destructor CSingleLock::~CSingleLock calls CSingleLock::Unlock which will call ReleaseSemaphore. It is hard to imagine how this could ever be right.
The basic rule is simple: NEVER USE THIS CLASS OR ITS RELATED CLASSES IN ANY REAL PROGRAM THAT MATTERS!
Does not contain a cross-reference to CSingleLock::Lock or CSingleLock::IsLocked. Does not mention that Unlock is incorrectly implemented and will not allow recursive acquisition of CMutex or CCriticalSection or sequential acquisition of CSemaphore or waiting for CEvent in a loop.
The example is complete trash. It shows a lock acquisition which does not check to see if the object has been locked, and then it shows a test on a CSingleLock object, which, if shared, would mean that the code is grossly incorrect, since there is a potential race condition. The example was written by someone who never understood synchronization, and is unrecoverable. Because CSingleLock::Lock is actually implemented incorrectly, there is no way it is even possible to write this code correctly!
In the article titled "Using CSpinButtonCtrl", no mention is made of the fact that a CSpinButtonCtrl will send up/down notifications via WM_HSCROLL or WM_VSCROLL messages (depending on the orientation ofo the control)
While the documentation of the constructor is slightly better than the documentation of the methods, there are still severe defects.
The proper prototypes are
CStdioFile(); CStdioFile(FILE * pOpenStream) throw(...); CStdioFile(LPCTSTR lpszFileName, UINT nOpenFlags) throw(...)
And the documentation should state that "CStdioFile will throw a CInvalidArgException * if the pOpenStream parameter is NULL, or if the lpszFileName parameter is NULL." and "CStdioFile will throw a CFileException * if a file name is given and there is an error opening the file"
It erroneously states that an open or creation file will throw a CFileException, which is a complete lie. It throws a CFileException *.
It fails to state that if the stream is provided from an existing FILE *, the stream will not be closed by CStdioFile::~CStdioFile. It also fails to state that CStdioFile::~CStdioFile will close a stream opened by CStdioFile::Open or CStdioFile(LPCTSTR, UINT) if the stream has not already been closed.
There is a possible implementation error; the code uses ^= to clear a bit when it should use &= ~. I find it a common programming error to use XOR when &~ is what is required; apparently nobody has taught these programmers that clearing a bit and complementing a bit are different concepts.
There is a hyperlink for stdio, but not for stdout or stderr. Why not? Even if they are all on the same page, there is no way the reader can a priori know that; or, alternatively, the hyperlink should be on the phrase "predefined input/output file pointers" (which should probably say "predefined standard handles" and indicate that these apply only to console applications).
It erroneously states the create and noInherit modes are optional; this is impossible because these modes do not exist. The correct documentation is that the modeCreate and modeNoInherit modes are optional. Or perhaps, explicitly, the CFile::modeCreate and CFile::modeNoInherit modes are optional, since that is how they would have to be written by the programmer.
The code example uses the now-obsolete TRY/CATCH macros instead of the modern C++ try/catch, and the obsolete char data type. It uses exit(1), a call that should never be made in any well-written program under any conditions whatsoever. It is a poor example of programming under any conditions. It gives a reason for the error only in debug mode, which is stupid, because in release mode it means that it fails silently, and worse still, merely exits the program without any indication to the user as to what went wrong. This is mind-bogglingly stupid. My most common description of these examples is "Where is the adult supervision?"
// examples for CStdioFile::CStdioFile
// This first example shows how to open a file explicitly using CFile::Open
CString FileName = _T("test.dat");
CStdioFile f1;
if( !f1.Open( FileName, CFile::modeCreate
| CFile::modeWrite | CFile::typeText ) )
{
// ...report error
// ...return control to caller or otherwise terminate flow of control
}
// We get here only if there was no open failure, so the above code must not just
// fall through on error
//====================================================================================
// This second example shows how to create a file from an existing stream
// This would be used in a console app, but not in a Windows program, where
// there is no stdout stream. It could be used for other FILE * streams in any program
CStdioFile f2( stdout );
//====================================================================================
// This third example shows how to create and open a file using the constructor
CString FileName = _T("test.dat");
try
{
CStdioFile f3( FileName,
CFile::modeCreate | CFile::modeWrite | CFile::typeText );
}
catch( CFileException * e )
{
// ... report error
e->Delete(); // unless exception is re-thrown
// ... return control to caller or otherwise terminate flow of control
}
This is not even documented, yet there is a very critical feature which must be documented!
The documentation should state "If the object was created with the CStdioFile(FILE *) constructor, the file will not be closed by the destructor. In all other cases, if there is an open file, it will be closed by the destructor.
This is undocumented. It should be documented, because it overrides the CFile::GetLength method but in fact this implementation is erroneous.
This is improperly implemented. It suffers from several bugs. Although it returns a ULONGLONG, in fact, it erroneously uses the FILE* stream method ftell, which only returns a LONG, and therefore will malfunction if the file is > 4.2GB. But in addition, it calls ftell, which has a serious implementation error. The consequence of this is that if it is applied to a Unix-formatted file (where \n is the only newline character, not \r\n as in Windows) it erroneously computes the current file position, then when "restoring" the file position, it restores it to the erroneously-computed value. This results in a disaster.
The correct documentation is
CStdioFile::GetLength
Obtains the current logical length of the file, in bytes.
virtual ULONGLONG GetLength( ) const;
Return Value
The length of the file, in bytes.
Remarks
This function will not work for text files whose length exceeds 232 bytes. It will return erroneous data.
This function will erroneously reposition the file pointer if the file in question is a Unix-formatted text file which only uses \n for line termination, instead of \r\n. It must not be used for such files.
This is undocumented. It should be documented, because it overrides CFile::GetPosition, but in fact this implementation is erroneous.
The correct documentation is
CStdioFile::GetPosition
Obtains the current value of the file pointer, which can be used in subsequent calls to Seek.
virtual ULONGLONG GetPosition ( ) const;
Return Value
The file pointer
Remarks
This function will not work for text files whose length exceeds 232 bytes. It will return erroneous data.
This function will erroneously compute the position if the file in question is a Unix-formatted text file which only uses \n for line termination, instead of \r\n. It must not be used for such files. Using the value returned for a Seek operation will position the file in the wrong position.
The method will throw exceptions when an error occurs, but this is documented only in the Remarks section. It also vaguely says "Will throw an exception", when the real documentation should say "Throws a CFileException *" with a hyperlink to CFileException. There is no statement that it will throw a CInvalidArgException * if the lpsz value is NULL. There is no See Also reference to CFileException or CInvalidArgException. There is a See Also reference to CArchive::ReadString but no See Also to CStdioFile::WriteString.
The function prototypes should be written as
virtual LPTSTR ReadString(LPTSTR lpsz, UINT max) throw(...); virtual BOOL ReadString(CString & s) throw(...);
The method will throw exceptions when an error occurs, but this is documented only in the Remarks section. It also vaguely says "throws an exception in response to several conditions, including the disk-full condition.", when the real documentation should say "Throws a CFileException * in response to several file system errors" with a hyperlink to CFileException. It does not state that it will throw a CInvalidArgException * if the lpsz value is NULL. There is no See Also reference to CFileException. There is a See Also reference to CArchive::ReadString but no See Also to CStdioFile::ReadString. There is no See Also link to either CFileException or CInvalidArgException.
The correct function prototype should be written
virtual void WriteString(LPCTSTR lpsz) throw(...)
Why does this not contain a hyperlink to CSimpleStringT in the See Also section?
This is generic to all CString constructors. If the initializing string is a MAKEINTRESOURCE of an integer value in the range of 1..65535, the constructor does a LoadString of that resource ID. Thus
CString t; t.LoadString(IDS_WHATEVER);
is the same as
CString t(MAKEINTRESOURCE(IDS_WHATEVER));
This is documented in my essay on CStrings.
While the documentation states explicitly that the upper bound of CTime is January 18, 2038 it does not state either the time on that date, or that the lower bound is 1-January-1970 00:00 (UCT)
This is specified as returning the ID for a button at a specified index. Unfortunately, if the index is out-of-range, it takes an ASSERT failure in the debug build and returns a meaningless value in both Debug and Release builds. In an intelligently-designed world, it would return a designated value if the item index were out-of-range. There is no mechanism I've found that actually reveals the number of items in a CToolBar, a grotesque oversight. The mechanism I had to use was:
BOOL CMyToolBar::IsValidIndex(UINT index)
{
TBBUTTON tb;
return (BOOL)SendMessage(TB_GETBUTTON, index, (LPARAM)&tb);
}
int CMyToolBar::GetItemCount(UINT index)
{
for(int i = 0; ; i++)
if(!IsValidIndex(i))
return i;
return 0;
}
Note that if the function _GetButton were to return a designated value instead of just blindly doing an ASSERT there would be no problem in having the user write an iterator across all buttons. The correct implementation of _GetButton (in MFC's bartool.cpp) should be
BOOL CToolBar::_GetButton(int nIndex, TBBUTTON* pButton) const
{
CToolBar* pBar = (CToolBar*)this;
if(!pBar->DefWindowProc(TB_GETBUTTON, nIndex, (LPARAM)pButton))
return FALSE;
// TBSTATE_ENABLED == TBBS_DISABLED so invert it
pButton->fsState ^= TBSTATE_ENABLED;
return TRUE;
}
In the topic labeled Tree Control Styles, the documentation erroneously states that
You can retrieve and change the styles after creating the tree control by using the GetWindowLong and SetWindowLong Windows functions, specifying GWL_STYLE for the nIndex parameter.
This is complete nonsense. The correct statement is
You can retrieve the styles of a control by using the CWnd::GetStyle method, or modify the styles by using CWnd::ModifyStyle.
The documentation for this function is incorrect, incomplete, and misleading.
It states
Return Value
The application's exit code; 0 means no errors, and values greater than 0 indicate an error. This value is used as the return value from WinMain.
Remarks
Do not call this member function from anywhere but within the Run member function.
The default implementation of this function writes framework options to the applications .INI file. Override this function to clean up when your application terminates.
There are many problems with this documentation; for example, it does not differentiate between applications and DLLs, and while it talks about "return values greater than 0", the type is int and it does not state what negative values mean.
The correct documentation is
There are two contents in which CWinApp::ExitInstance will be called: applications and DLLs.
Return Value:
For applications: the applications' exit code; 0 means no errors, and values other than 0 indicate an error. The value returned is the value which becomes the return value from WinMain.
For DLLs: the return value is ignored.
Remarks
For applications, this is called from the CWinApp::Run method. You must not call it explicitly at any time for any reason with one exception: If you override the Run method in your derived class, you must call this method before the Run method returns.
For DLLs: this is called in response to the DLL_PROCESS_DETACH event from DllMain. As such, what you do here is subject to the restrictions of DllMain.
The default implementation of this function writes framework options to the applications profile. This is stored in the Registry under the key HKEY_CURRENT_USER\Software\profilename where profilename is the name derived from the m_pszProfileName string.
Note that this behavior should not be confused with the behavior of CWinThread::ExitInstance for threads.
Note that this documentation erroneously talks as if .INI files still work, and are used, instead of taking into account the fact that Win32 redirects private profile string output to a Registry key. This documentation should be brought up to standards of behavior for Win32, and not talk as if it is describing the first MFC implementation done for 16-bit Windows!
This documentation is meaningless gibberish, left over from 16-bit Windows. It is completely irrelevant to modern Win32 programming practice.
For example, it says
Application initialization is conceptually divided into two sections: one-time application initialization that is done the first time the program runs, and instance initialization that runs each time a copy of the program runs, including the first time. The framework's implementation of WinMain calls this function.
This is complete nonsense. It has never made sense in Win32, and in fact it erroneously is a direct copy of the dead Win16 documentation. The correct documentation is
Application initialization is done by the framework calling InitInstance. Any initialization required is performed in this method.
There are two situations in which a derived class from CWinApp is used, and the meaning of InitInstance is different in each case.
In an application class, there is always exactly one instance of the CWinApp-derived class, known as theApp, and it is called from WinMain.
In a DLL, there is always exactly one instance of the CWinApp-derived class, known as theApp, but it actually represents the DLL instance. The InitInstance method is called in response to the DLL_PROCESS_ATTACH event from DllMain, and therefore is subject to all the limitations of the DLL_PROCESS_ATTACH event and the context of DllMain. Consult the DllMain documentation for the details. Note that you may not start a thread in the InitInstance handler for a DLL.
See also CWinApp::ExitInstance.
Note that this should not be confused with the behavior of CWinThread::InitInstance in the case of UI threads.
Note that this documentation erroneously talks as if .INI files still work, and are used, instead of taking into account the fact that Win32 redirects private profile string output to a Registry key. This documentation should be brought up to standards of behavior for Win32, and not talk as if it is describing the first MFC implementation for 16-bit Windows!
This documentation is another screamingly bad example of a collection of confusing, mis-stated, and erroneous facts, failures to explicitly state best practice, and failures to clarify serious issues.
The correct documentation is
The main thread of execution of an application in MFC is contained in the CWinApp class, and for any real application, from an application-specific derived class of CWinApp. In addition to the CWinThread functionality, CWinApp introduces other features such as reading and storing program options. A multithreaded program will have additional instances of CWinThread, one for each thread.
There are two general types of threads that CWinThread supports: worker threads, and what are called "user-interface" threads, which is misleading designation; what it really means is "a thread with a message pump". It is generally considered poor practice to put visible user-interface objects into any thread other than the main UI thread (the one that is running from the CWinApp-derived class) and to attempt to do so without understanding the extremely complex rules that must be followed for correct behavior will result in a program which will exhibit severely incorrect behavior. Worker threads do not have a message pump. Secondary UI threads are useful and in some cases critical, but should not actually have user interface objects in them. The name is historical and consequently very confusing.
Objects of class CWinThread by default exist only for the duration of the thread. To cause the lifetime to be extended beyond the life of the thread, the thread must be created using AfxBeginThread with the CREATE_SUSPENDED flag, the m_bAutoDelete flag must be set to FALSE, and then ResumeThread should be called. It is erroneous to attempt to modify the m_bAutoDelete flag of a thread that is actually running.
The CWinThread class is required to make MFC fully thread-safe. In addition, you can use your CWinThread-derived class to hold your own thread-specific information.
Only threads controlled by a CWinThread object may use MFC objects. In an MFC application, a thread should never be created by using CreateThread, _beginthread, or _endthread.
It is inappropriate to ever call ExitThread,, _endthread, _endthreadex, or AfxEndThread from within any CWinThread context. This will almost always result in seriously erroneous behavior. No program that does this can be considered robust or correct.
Note that the inclusion of a method in a CWinThread class does not mean that the code of that method executes in the context of the thread. Code which is called is always executed in the context of the calling thread; thus, any method called from a given CWinThread-managed thread will be executed in the context of that thread, whether the code is part of the CWinThread class or part of some other class. Similarly, other threads are free to call methods of the CWinThread-derived class, and that code is always executed in the context of the calling thread. Note that CWinThread provides no thread safety for objects other than those internal to the MFC framework. To protect your own data, including MFC collections C++ Standard Library collections, members of your CWinThread-derived class, data in any other class read or modified by your thread, you, and you alone, are responsible for providing synchronization mechanisms, such as mutex or CRITICAL_SECTION, or the use of Interlocked... primitives, to control access to that information.
To create a thread, use AfxBeginThread. There are two forms of this method, one for worker threads and one for UI threads. For UI threads, the CRuntimeClass pointer to the CWinThread-derived class is passed in. This creates a thread with a message pump. This means that the thread can handle messages to windows it owns, or thread messages (via PostThreadMessage). A UI thread may often have windows, which are used as message sinks. However, these windows must be invisible, owned, top-level windows, not visible windows. Many components of Windows require that the thread which invokes operations be running in a thread with a message pump, such as CAsyncSocket and SAPI (Speech API) calls. However, it is always considered bad practice to create user-visible windows from a thread, including any form of a MessageBox call.
Instead of calling AfxBeginThread, the behavior of AfxBeginThread can be simulated by constructing a CWinThread object on the heap, then calling CWinThread::CreateThread. This two-stage construction method is useful if you wish to retain the CWinThread-derived object and create a new thread after a previous thread has terminated.
InitInstance is overridden in a UI thread to provide any required per-thread initialization.
Remarks:
Typically, you override InitInstance to perform any tasks that must be completed when a thread is first created. Note that upon successful return, the message pump will run.
Note that there is no mechanism for passing parameters into a UI thread. The normal method of "passing parameters" into a UI thread can be accomplished by one of several mechanisms:
Supply the CREATE_SUSPENDED flag on AfxBeginThread. When the CWinThread-derived object is returned, member values may be set either directly (if public) or via methods (if protected). When CWinThread::ResumeThread is called, the values will be available to the InitInstance method.
Use new to create an instance of the CWinThread-derived class. You can either add additional parameters to the constructor, or, after creation, set values either directly (if public) or via methods (if protected). Then use CWinThread::CreateThread to create the thread; the values will be available to InitInstance.
Do not bother to set values, and do not use the in InitInstance. Instead, create Message Map entries for ON_THREAD_MESSAGE or ON_REGISTERED_THREAD_MESSAGE, and use PostThreadMessage to pass information into the thread.
Note that it is not possible to PostThreadMessage to a UI thread until the Run method has executed a GetMessage or PeekMessage call to create the message queue. Thus, the following code cannot work:
CWinThread * thread = AfxBeginThread(RUNTIME_CLASS(CMyThreadClass));;
thread->PostThreadMessage(user_defined_message_id, wparam_value, lparam_value);
because it is unpredictable when the thread will actually have created its message queue. It is always an error to do a Sleep() call in the hopes that the randomly-chosen interval will be meaningful and allow time for the thread to start running. The only way this can be handled is by starting the thread and waiting for the thread to indicate that it now has a thread queue. One method that accomplishes this is to explicitly do a ::PeekMessage to force the message queue to exist, then notify the creating thread that this has occurred.
CMyThreadClass * thread = (CMyThreadClass *)AfxBeginThread(RUNTIME_CLASS(CMyThreadClass), THREAD_PRIORITY_NORMAL, 0, CREATE_SUSPENDED); thread->target = this; thread->ResumeThread();
then have a user-defined message handler in the class which is the target, such as
ON_REGISTERED_MESSAGE(message_id_here, OnThreadRunning)
In the InitInstance handler do
BOOL CMyThreadClass::InitInstance()
{
MSG msg;
::PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE);
target->PostMessage(message_id_here, (WPARAM)this);
return TRUE;
}
In the target class, the handler is
LRESULT targetclass::OnThreadRunning(WPARAM wParam, LPARAM)
{
CMyThreadClass * thread = (CMyThreadClass *)wParam;
thread->PostThreadMessage(user_defined_message_id, wparam_value, lparam_value);
return 0;
}
This call does not work correctly when applied to a dialog with a menu. The calculated window rectangle does not take into account the height of the menu bar. Therefore, after doing CalcWindowRect, you must compensate for the error, as shown:
CRect r; ...set desired client rectangle CalcWindowRect(&r); r.bottom += ::GetSystemMetrics(SM_CYMENU);
the general solution would be
CRect r; ...set desired client rectangle CalcWindowRect(&r); r.bottom += (GetMenu() != NULL ? ::GetSystemMetrics(SM_CYMENU) : 0);
Submitted by reader Jim Beveridge
You list the solution as
r.bottom += ::GetSystemMetrics(SM_CYMENU);
This is actually not a complete solution because the menu will take multiple lines if the window is resized small horizontally. I solved this problem years ago, but the solution was ugly. I think I retrieved the client rect and then kept adding ::GetSystemMetrics(SM_CYMENU) until everything added up… But I don’t remember exactly.
This does not specify that the lpszWindowName parameter can be NULL.
If the window style is given as WS_POPUP, the hwndParent parameter is ignored, and the parent returned by ::GetParent is not the parent of the window, but the top-level window of the application. This can be verified by using Spy++ to inspect the parent link, which is indeed the handle to the top-level window, not the handle that was passed to ::CreateWindow.
This does not specify that the lpszWindowName parameter can be NULL.
If the window style is given as WS_POPUP, the hwndParent parameter is ignored, and the parent returned by ::GetParent is not the parent of the window, but the top-level window of the application. This can be verified by using Spy++ to inspect the parent link, which is indeed the handle to the top-level window, not the handle that was passed to ::CreateWindowEx.
Does not exist! Why doesn't this method exist? There is a CWindow::DeferWindowPos, so why isn't there a CWnd::DeferWindowPos?
This message is not supported by all window classes; in particular, it is not supported by classes that are derived directly from CWnd. It is also not supported by certain variants of CStatic. (For more details, see CWnd::SetFont)
This does not have See Also references to CWnd::InvalidateRect or CWnd::InvalidateRgn
This does not have a See Also to CWnd::InvalidateRgn, or CWnd::Invalidate.
This does not have a See Also to CWnd::InvalidateRect or CWnd::Invalidate
The documentation is incomplete and incoherent. It starts off by saying, in the Remarks, that "Most controls send this message to their parent (usually a dialog box)..." whereas the correct statement is "The standard controls send this message to their parent". It also says, in the earlier description, that CTLCOLOR_STATIC is sent by a static control. The correct statement is
It does not mention that if the HBRUSH is NULL, any changes made to the pDC are discarded. It does not hint at the fact that in MFC, the brush must be allocated in the class that contains the handler, and must not be allocated locally.
There are no hyperlinks to the WM_xxxCTLCOLOR messages
There is no option in the Properties for creating a handler for this message.
The message has a BOOL return type but nothing is said about this return type or its significance. Note that while WM_DEVICECHANGE has two possible values, TRUE and BROADCAST_QUERY_DENY, there is no reference to the message or the return values in this discussion.
At no point is it mentioned that these messages can come from a spin button control or track bar control. In fact, there are no references to these controls in the See Also section.
It does not mention that for spin buttons, the nSBCode is SB_THUMBPOSITION and that the nPos parameter is valid for controls whose range is < 16 bits.
At no point is it mentioned that to test for messages from a track bar control or spin button control, the pScrollBar can be checked against a control variable, e.g., it is possible to write
if(pScrollbar != NULL && pScrollBar == (CScrollBar *)c_MySpinControl)
..
(It is not obvious to a beginner that this is even possible, let alone sensible)
There was an unfortunate and undocumented (and probably unnecessary) change in the return type of the CWnd::OnNcHitTest method. Note that although the header files changed with VS2005, the documents for VS2005 are incorrect; the documentation error was corrected in VS2008. This means that if you upgrade an existing project to VS2005, they will not compile; if you make the upgrade, you can't build under earlier versions. The compatibility change I made was to modify the header file and implementation file as shown below. Now the code will compile under all versions of VS.
The error is
č ON_WM_NCHITTEST() 1>filename(ln) : error C2440: 'static_cast' : cannot convert from 'UINT (__thiscall yourclass::* )(CPoint)' to 'LRESULT (__thiscall CWnd::* )(CPoint)' 1> Cast from base to derived requires dynamic_cast or static_cast
Create a header file that specifies the following:
#if _MFC_VER < 0x0800 typedef UINT NCHITTEST_RESULT ; #else typedef LRESULT NCHITTEST_RESULT ; #endif
In the header file for your class, do a #include of the header file you just created, and modify the declaration of the OnNcHitTest handler to be
... NCHITTEST_RESULT OnNcHitTest(CPoint point);
in the implementation file
NCHITEST_RESULT YourClassNameHere::OnNcHitTest(CPoint point)
{
...
}
There is also an error in the implementation of WM_NCHITTEST. Normally, if you want to do something like disable the ability to drag a window, you might check the return value from the superclass, and if it is HTCAPTION, return HTNOWHERE. This does not work on Vistra if Aero is enabled. Why this should be so is a mystery, and why it is not documented is more of a mystery. It is not clear what other erroneous behavior occurs with this handler if Aero is enabled. Suggestion to Microsoft: have the implementers read the specifications before writing the code!
The documentation contains the statement
If nHitTest is HTERROR and message is a mouse button-down message, the MessageBeep member function is called.
Huh? When? How? Does this mean to say something like this?
If you return FALSE from this method, and nHitTest is HTERROR and message is a mouse button-down message, the default action is that the MessageBeep member function is called with the parameter <undefined parameter, please supply>.
There are serious implications to how this is called. The PreSublcassWindow virtual method is called from two different locations in the framework. One of them is from the DDX_Control handler, indirectly, when it calls SubclassWindow. In this case, the window already exists. So you can pretty much do whatever you want in the PreSubclassWindow handler.
But a more serious problem arises if you create the window dynamically by calling CWnd::Create on a variable which is a CWnd-derived type. In this case, a hook is set which is a CBT_HOOK and which intercepts the window creation event. In this case, you are inside a hook-handler. The MFC framework calls PreSubclassWindow from the _AfxCbtFilterHook function in wincore.cpp. The problem here is that if you attempt to do a CreateWindow API (which means calling either Create or CreateEx for any CWnd-derived class, by any number of levels of indirection), this will cause a recursive call on the hook function, and MFC will not allow this. It therefore triggers an assertion failure. However, for reasons that are not fully comprehensible, instead of generating a sane error message, this generates an "unhandled exception: breakpoint" failure. The correct behavior would be to pop up an assertion-failure message when an assertion fails, but that is not what happens, even in debug mode.
This does not occur if you are attempting to create a window in PreSubclassWindow when an existing window (such as a dialog or CFormView control, as the most common example) is being sublcassed.
This restriction is not documented in the almost-nonexistent PreSubclassWindow documentation.
The solution is to not attempt to create another window in the PreSubclassWindow handler.
Must add the information:
Not all window classes support the WM_SETFONT message. In particular, although this is listed as a member of the CWnd class, a class which is derived directly from CWnd will not support WM_SETFONT, and certain variants (undocumented) of CStatic, including static controls with the style bits SS_xxxFRAME or SS_xxxRECT. Therefore a set of handlers must be declared, and a member variable created to hold the font information.
class CMyWindow : public CWnd {
protected:
afx_msg LRESULT OnSetFont(WPARAM, LPARAM);
afx_msg LRESULT OnGetFont(WPARAM, LPARAM);
HFONT m_font;
...
};
The following declarations must be added to the message map of your class:
ON_MESSAGE(WM_SETFONT, OnSetFont) ON_MESSAGE(WM_GETFONT, OnGetFont)
and the following handlers must be added
LRESULT CMyWindow::OnSetFont(WPARAM wParam, LPARAM lParam)
{
m_font = (HFONT)wParam;
if(LOWORD(lParam))
{
Invalidate();
UpdateWindow();
}
return 0;
}
LRESULT CMyWIndow::OnGetFont(WPARAM, LPARAM)
{
return (LRESULT)m_font;
}
The documentation states
When used with .MODEL, starts a near data segment for initialized data (segment name _DATA)
I have no idea what this means. What, exactly, is a "near" segment in Win32 or Win64? There is nothing under .MODEL to suggest that this has any meaning.
The documentation states
When used with .MODEL, starts a near data segment for uninitialized data (segment name _BSS)
I have no idea what this means. What, exactly, is a "near" segment in Win32 or Win64? There is nothing under .MODEL to suggest that this has any meaning.
The description of this is rubbish. It says
Can be used to define data such as BYTE.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DB crosslink to anything. The only appearance of "db" at all on the page is the part where it asks for feedback! So how is this usable?
Now, if it duplicated the documentation of BYTE, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define byte data. This is a synonym for BYTE. It allocates and optionally initializes 1 byte of storage for each initializer.
[[name]] DB iniitalizer [[, initializer]]
See Also
Other Resources
Directives Reference
It does not mention that the acceptable range of values for the initializer is -128..255.
This same error occurs in the DD, DF, DQ and DW documentation. Note that the syntax of name and initializer is not specified anywhere in the documentation.
The description of this is rubbish. It says
Can be used to define data such as DWORD.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DD crosslink to anything. So how is this usable?
Now, if it duplicated the documentation of DWORD, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define doubleword data. This is a synonym for DWORD. It allocates and optionally initializes 4 bytes of storage for each initializer.
[[name]] DD iniitalizer [[, initializer]]
See Also
Other Resources
Directives Reference
Does not have a hyperlink to SDWORD or DWORD.
Perhaps because there is no cross-link to the concept of initializer, it is not at all clear that an initializer for DD can be a floating point number, and therefore it is also able to initialize the equivalent of the C/C++ float with an initialization clause.
It does not mention the integer range is -2147483648..4294967295.
This same error occurs in the DB, DF, DQ and DW documentation. Note that the syntax of name and initializer is not specified anywhere in the documentation.
Why does this not have hyperlinks to /MDd, /MLd and /MTd? Why just a hyperlink to /LDd?
This is rather important but its proper use is not documented. In fact, the macro itself is not documented.
It is polymorphic in that the same DEFINE_GUID declaration is used both to define the GUID and to reference the GUID. By default, it references the GUID.
To actually create a GUID of your own, you must
// {2852F987-BDE4-48ad-A36C-D60BC9F52E9C}
DEFINE_GUID(myguid,
0x2852f987, 0xbde4, 0x48ad, 0xa3, 0x6c, 0xd6, 0xb, 0xc9, 0xf5, 0x2e, 0x9c);
#define INITGUID #include <guiddef.h> #include "myguid.h"
The above is the source file in its entirety.
Note that you must select "All Configurations" as shown below
You may now use #include "myguid.h" in any other compilation unit and you will have a GUID defined.
#include "stdafx.h"
#include <guiddef.h>
#include "myguid.h"
BOOL SomeFunction()
{
const GUID * guid = &myguid;
...
}
The declaration for DeviceIoControl erroneously gives the parameters as
BOOL WINAPI DeviceIoControl( __in HANDLE hDevice, __in DWORD dwIoControlCode, __in LPVOID lpInBuffer, __in DWORD nInBufferSize. __out LPVOID lpOutBuffer, __in DWORD nOutBufferSize, __out LPDWORD lpBytesReturned, __in LPOLVERLAPPED lpOverlapped);
but this is simply incorrect. In a sane world, the declaration would be
BOOL WINAPI DeviceIoCotnrol( __in HANDLE hDevice, __in DWORD dwIoControlCode, __in_opt_bcount(nInBufferSize) LPVOID lpInBuffer, __in DWORD nInBufferSize. __out_opt_bcount(nOutBufferSize) LPVOID lpOutBuffer, __in DWORD nOutBufferSize, __out LPDWORD lpBytesReturned, __in_opt LPOVERLAPPED lpOverlapped);
and that is just to be consistent with the erroneously-defined API; the lpInBuffer should be an LPCVOID since it only needs to be const!
The failure to define these parameters correctly generates "false positives" when /analyze is added to the compilation options in VS2008.
I suspect that far many more APIs are not annotated correctly with respect to /analyze.
The description of this is rubbish. It says
Can be used to define data such as FWORD.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DF crosslink to anything. So how is this usable?
There is also no explanation of why 6-byte blocks of data have any use.
Now, if it duplicated the documentation of FWORD, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define six-byte data values. This is a synonym for FWORD. It allocates and optionally initializes 6 bytes of storage for each initializer.
[[name]] DF iniitalizer [[, initializer]]
See Also
Other Resources
Directives Reference
This same error occurs in the DB, DD, DQ and DW documentation. Note that the syntax of name and initializer is not specified anywhere in the documentation.
The documentation is confusing and misleading.
It starts with the description:
This can reduce the size of the working set for some applications.
A more precise description would be
This means that DllMain will not be called whenever a thread is created or destroyed. This will improve performance in several ways: it means that useless calls will not be made during thread creation or termination, that the possibility of taking page faults in order to page in the code for DllMain is reduced to zero, and this may additionally result in reducing the working set size of an application.
Then it provides very confusing information about hModule:
A handle to the DLL module for which the DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications are to be disabled. The LoadLibrary, LoadLibraryEx or GetModuleHandle function returns this handle. Note that you cannot call GetModuleHandle with NULL because this returns the base address of the executable image, not the DLL image.
Besides the obvious failure to match plurality of subject and object, the information is very confusing. Where do we know that the GetModuleHandle, which returns an object of type HMODULE, is giving a base address? What does this have to do with anything? In fact, using the handle provided by LoadLibrary or LoadLibraryEx would be the height of folly, because it says that the loader of the DLL is allowed to disable thread library calls, which would be a design blunder of immense proportions! How could the code that loads a DLL possibly know if it is correct to disable thread callbacks? And it is even worse for GetModuleHandle, which says that any random piece of code is permitted to disable thread callbacks!!!! Who writes this crap????
The only sensible way to allow thread callbacks to be disabled is if the author of the DLL writes this in DllMain, so the correct documentation would be
This function should only appear in the DLL_PROCESS_ATTACH handler code in DllMain and the parameter provided is the HMODULE parameter of DllMain. It should never be called for any DLL by code that is not part of the DLL, and calling it from anywhere other than the DLL_PROCESS_ATTACH handler introduces possibilities of serious errors under maintenance.
It goes on to add
Do not call this function from a DLL that is linked to the static C run-time library (CRT). The static CRT requires DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications to function properly.
The correct documentation would be
Do not call this function from a DLL that is linked to the static C multithreaded run-time library (MT-CRT). The static multithreaded CRT requires DLL_THREAD_ATTACH and DLL_THREAD_DETACH notifications to function properly (note that these notifications are processed in the CRT code that is executed before DllMain is called). It is erroneous to use a DLL that has the static C single-threaded run-time library (ST-CRT) in a DLL that can be potentially called from multiple threads. It should be assumed that every DLL is potentially used in a multithreaded environment and if static linking is used, the static single-threaded CRT must never be considered.
This is a vague error code. At least one cause of this error code is to call an automation interface to open an Office file and give it a nonexistent filename. The error that comes back from the Invoke method is DISP_E_EXCEPTION. Unfortunately, the MFC handler, OleDisp2.cpp, in the code in the function COleDispatchDriver::InvokeHelperV, detects this error, and eventually calls the following code:
if (vtRet != VT_EMPTY)
{
// convert return value
if (vtRet != VT_VARIANT)
{
SCODE sc = VariantChangeType(&vaResult, &vaResult, 0, vtRet);
if (FAILED(sc))
{
TRACE(traceOle, 0, "Warning: automation return value coercion failed.\n");
VariantClear(&vaResult);
AfxThrowOleException(sc);
}
ASSERT(vtRet == vaResult.vt);
}
The values passed to VariantChangeType are (VT_EMPTY, VT_EMPTY, 0, 0x0009), which are apparently unacceptable to the VariantChangeType function, which returns the error code DISP_E_TYPEMISMATCH, which is then erroneously returned to the caller as if it were the actual error incurred!
See DISP_E_EXCEPTION for an explanation of why this value is erroneously returned from some calls, in particular, Open calls for opening Office documents via automation!
This says it is defined only when the multithreaded DLL option is specified. It should add
This applies to the selection of the C runtime. Note that _MT is also defined.
and the _MT shall be a hyperlink to that actual entry. There should also be an observation that there is no single-threaded DLL option selectable.
Does not mention the implication of how DLL_THREAD_DETACH works: a DLL can receive DLL_THREAD_DETACH notifications for threads for which it never received DLL_THREAD_ATTACH notifications; if the threads existed before the DLL was loaded, no notifications will take place.
Does not mention that CreateThread, LoadLibrary or LoadLibraryEx calls cannot be executed in DllMain.
Makes the truly embarrassing statement that "Unfortunately, there is not a comprehensive list of safe functions in Kernel32.dll". Seriously, guys, how hard is it to create this list? There are a finite number of APIs in Kernel32.dll, How hard is it, really, to have someone check those out and make the list? There is really no valid excuse for not having done this 20 years ago; to have gone this long without it is simply irresponsible!
This does not work correctly. When used in mode GM_ADVANCED and when there is scaling, there is a serious roundoff error In is also inconsistent with the behavior not using GM_ADVANCED, and is inconsistent with the behavior as specified in the documentation.
When I used GM_ADVANCED mode, and set the scaling factor to 20, each pixel in the original bitmap displays as a 20-pixel square. That's as expected. But when I put the mouse over the window, I get points in device coordinates in the client space. That's as expected. But I want to convert these to logical coordinates. The obvious solution would be to call DPtoLP, but it gives an erroneous answer. I found similar roundoff errors at a scaling factor of 5. There is different erroneous behavior if I do not use GM_ADVANCED and just use SetMapMode(MM_ISOTROPIC) with SetWindowExt and SetViewportExt.
These are summarized in the table below:
|
x |
20× | 5× | ||||
| expected | GM_ADVANCED | MM_ISOTROPIC | expected | GM_ADVANCED | MM_ISOTROPIC | |
| 0 | 0 | -1 | 0 | 0 | 0 | 0 |
| 1 | 0 | -1 | 0 | 0 | 0 | 0 |
| 2 | 0 | -1 | 0 | 0 | 0 | 0 |
| 3 | 0 | -1 | 0 | 0 | 1 | 1 |
| 4 | 0 | -1 | 0 | 0 | 1 | 1 |
| 5 | 0 | -1 | 0 | 1 | 1 | 1 |
| 6 | 0 | -1 | 0 | 1 | 1 | 1 |
| 7 | 0 | -1 | 0 | 1 | 1 | 1 |
| 8 | 0 | 0 | 0 | 1 | 2 | 2 |
| 9 | 0 | 0 | 0 | 1 | 2 | 2 |
| 10 | 0 | 0 | 0 | 2 | 2 | 2 |
| 11 | 0 | 0 | 1 | 2 | 2 | 2 |
| 12 | 0 | 0 | 1 | 2 | 2 | 2 |
| 13 | 0 | 0 | 1 | 2 | 3 | 3 |
| 14 | 0 | 0 | 1 | 2 | 3 | 3 |
| 15 | 0 | 0 | 1 | 3 | 3 | 3 |
| 16 | 0 | 1 | 1 | 3 | 3 | 3 |
| 17 | 0 | 1 | 1 | 3 | 3 | 3 |
| 18 | 0 | 1 | 1 | 3 | 4 | 4 |
| 19 | 0 | 1 | 1 | 3 | 4 | 4 |
| 20 | 1 | 1 | 1 | 4 | 4 | 4 |
This erroneous behavior is not documented anywhere, and most especially it is not documented in the DPtoLP documentation. And frankly, it doesn't make any sense, by any stretch of the imagination.
The description of this is rubbish. It says
Can be used to define data such as QWORD.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DB crosslink to anything. The only appearance of "db" at all on the page is the part where it asks for feedback! So how is this usable?
Perhaps because there is no cross-link to the concept of initializer, it is not at all clear that an initializer for DQ can be a floating point number, and therefore it is also able to initialize the equivalent of the C/C++ double with an initialization clause.
Now, if it duplicated the documentation of QWORD, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define byte data. This is a synonym for QWORD. It allocates and optionally initializes 8 bytes of storage for each initializer.
[[name]] DQ iniitalizer [[, initializer]]
See Also
Other Resources
Directives Reference
It does not mention that the allowable integer range is -9223372036854775808..18446744073705551615.
The same error exists for DB, DF, DD, and DW. Note that the syntax of name and initializer is not specified anywhere in the documentation.
The idAni parameter is said to specify the "type of animation", and says if you specify IDANI_CAPTION, certain effects will be seen. But the description suggests there are other possible values that might have other effects, but what these values might be and what these effects are is not specified.
It does not specify in the Remarks section (and there isn't a Remarks section) that this will have no effect if the user has selected the option to have no animations displayed.
Some documentation erroneously states that if the nCount (DrawText) or cchText (DrawTextEx) parameter is 1, it assumes the string is null-terminated. This is incorrect. The nCount/cchText parameter should be -1 to indicate the string is null-terminated. This is correct in the discussion of CDC::DrawText. This has been fixed in later versions.
In addition, for what is apparently backward-compatibility, the second parameter, the string pointer, is erroneously give as LPCTSTR, in spite of the fact that under some conditions this cannot work at all.
The erroneous documentation says:
lpString
[in] Pointer to the string that specifies the text to be drawn. In the nCount parameter is -1, the string must be null-terminated.
If uFormat includes DT_MODIFYSTRING, the function could add up to four additional characters to this string. The buffer containing the string should be large enough to accommodate these extra characters.
Unfortunately, the correct documentation should say
lpString
[in/out] NOTE: ALTHOUGH THIS IS SPECIFIED AS AN LPCTSTR, THIS IS ERRONEOUS AND IS MAINTAINED FOR BACKWARD COMPATIBILITY WITH EXISTING CODE. IT IS NOT ACTUALLY CORRECT BECAUSE THIS PARAMETER CAN BE AN 'OUT' PARAMETER AS WELL.
If DT_MODIFYSTRING is not specified, this parameter can point to a constant string. However, if DT_MODIFYSTRING is specified (along with either DT_END_ELLIPSIS or DT_PATH_ELLIPSIS) the function will modify the string, in which case (a) it cannot be a pointer to a constant string, or an access fault will occur, and (b) since the modification can add up to four characters to the string, the buffer must be at least four characters longer than the input text to allow for this modification. Under these conditions, the parameter should be treated as an LPTSTR, not as an LPCTSTR. Note that if the buffer is not properly allocated so that it can accommodate the additional characters, the integrity of the program is compromised and the program can crash, and in extreme cases the security of the system running the code can be compromised.
NOTE THAT WHEN EXTENDED COMPILER CHECKING (/W4 or /WALL), OR /ANALYZE ARE USED, THE COMPILER OR PREFAST WILL ERRONEOUSLY ALLOW A CONSTANT STRING TO BE PASSED IN EVEN IF DT_MODIFYSTRING IS SPECIFIED. THE RESULT WILL BE CODE WHICH CAN FAIL AND POTENTIALLY PRESENT A SECURITY HAZARD.
(Thanks to Giovanni Dicanio for pointing out an ongoing discussion in another newsgroup)
Note that in my rewrite I replaced the wishy-washy "should be" large enough with the much more forceful "must be" large enough, and emphasized the "must".
The description of this is rubbish. It says
Can be used to define data such as TBYTE.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DT crosslink to anything. So how is this usable?
Now, if it duplicated the documentation of TBYTE, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define byte data. This is a synonym for TBYTE. It allocates and optionally initializes 10 bytes of storage for each initializer.
[[name]] DT iniitalizer [[, initializer]]
This can be used to define 10-byte floating point types, the equivalent of what the C/C++ compiler would have done if it supported the long double data type.
A floating point number can specify <<value here>> digits of precision, and the allowable range for a floating point number is 3.37×10-4392..1.18×104392.
See Also
Other Resources
Directives Reference
It does not mention that the acceptable range of values for the initializer is -128..255.
This same error occurs in the DD, DF, DQ and DW documentation. Note that the syntax of name and initializer is not specified anywhere in the documentation.
The description of this is rubbish. It says
Can be used to define data such as WORD.
DB
See Also
Other Resources
Directives Reference
Now, it is not clear how this documentation is even vaguely usable. For example, the Directives Reference hyperlink returns to a page in which there is no DB crosslink to anything. The only appearance of "db" at all on the page is the part where it asks for feedback! So how is this usable?
Now, if it duplicated the documentation of WORD, it might be useful. For example, if it was described as shown below, it would indicate that some intelligence had been used in writing the description.
Used to define byte data. This is a synonym for WORD. It allocates and optionally initializes 2 bytes of storage for each initializer. For signed values, the SWORD declaration can be used.
[[name]] DW iniitalizer [[, initializer]]
See Also
Other Resources
Directives Reference
The same error exists for DB, DD, DF, and DQ. Note that the syntax of name and initializer is not specified anywhere in the documentation.
Contains no hyperlink to SDWORD. Does not contain a hyperlink to DD.
Does not state that the allowable range is -2147483648..4294967295. Does not indicate if floating-point initializers are valid.
This is said to be the same as %OUT, but %OUT is said to be the same as ECHO, but they both have different syntax. It makes no sense.
Unfortunately, the documentation is also confusing. The syntax for ECHO is
ECHO message
but the syntax for %OUT is
%OUT
That is, it has no message parameter. Either they are synonyms, or they are different, and if they are the same, they would have the same syntax. Doesn't anyone actually read this documentation to see if it makes sense?
This has to be one of the worst designs I have seen recently; what is more unforgivable is that it was designed recently, and is not a throwback to Win16 and some brain-dead design decisions.
In an intelligent world, the design would have been that if WPARAM, LPARAM were specified as 0,0 it would return the size of the buffer required (including the terminal NUL character) and if they were non-0 it would return the count of characters actually returned (not including the NUL character). Proving once again that software evolves, because there is no trace of intelligent design to be found here.
It is not clear why an ANSI version of this is not supported.
EM_GETCUEBANNER does not contain a hyperlink to EM_SETCUEBANNER.
See also CEdit::GetCueBanner
Does not contain a hyperlink to EM_GETCUEBANNER.
This does not exist. Only EMRCOLORMATCHTOTARGETW exists.
This is missing a hyperlink to LOGCOLORSPACE. In addition, it is almost certainly not a LOGCOLORSPACE value, which has a TCHAR field; the type of the field would depend upon the creator, not the reader, and consequently, it is most likely that it is a LOGCOLORSPACEA, but this is unspecified.
The offPalEntries says, vaguely, "offset to palette entries". The correct documentation is, "The offset in bytes, from the beginning of the structure, to an array of PALETTEENTRY values. Note that the offset might be nonzero even if nPalEntries is 0."
The nSizeLast documentation is a bit vague, and the documentation would be helped by a picture, e.g.,
+----------------+ - - - - - - - - - - - - - - - - - - - - - - - -
| EMR_EOF | ^ ^
+----------------+ | |
| .nSize | | |
+----------------+ | offPalEntries |
| .nPalEntries | | | offPaleEntries +
+----------------+ | | nPalEntries * sizeof(PALETTEENTRY)
| .offPalEntries | V |
+----------------+ - - - - - - - - |
| | \ |
: : > .nPalEntries * sizeof(RGBQUAD) |
| | / V
+----------------+ - - - - - - - - - - - - - - - - - - - - - - - -
| .nSizeLast |
+----------------+
This documentation is confusing and erroneous. The specification of the Ver[1] field is "Pointer to an array of TRIVERTEX structures that each define a triangle vertex". This doesn't make any sense at all, because it is most definitely not a pointer. It is the first element of a TRIVERTEX array. A TRIVERTEX most definitely does not define "a triangle vertex", it defines a vertex, which may also be a rectangle vertex.
In the remarks it states "The nVer element designates the beginning of the variable-length area"; the nVer element does no such thing; it is actually the count of the number of vertices. The correct documentation is "The Ver element designates the beginning of the variable-length area". The documentation also erroneously refers to the ulMode field as the jlMode field. The documentation does not specify what the ulMode actually is; it should explicitly state that the ulMode is one of GRADIENT_FILL_H, GRADIENT_FILL_V, or GRADIENT_FILL_TRIANGLE.
The documentation would be immensely helped by a picture
+--------------+ | EMR emr | : : | | +--------------+ | DWORD nVer | +--------------+ | DWORD nTri | +--------------+ +---+ ULONG ulMode +---------------------------------------------------+ | +--------------+ +--------------+ - - - - - - - - - - - - - - | - - - | | TRIVERTEX | | TRIVERTEX[0] | \ | ^ | | Ver[1] | | | | | | | +--------------+ +--------------+ | | | | | TRIVERTEX[1] | | | | | | | | | | | +--------------+ | | | | | | > nVer | | ((nVer - 1) * sizeof(TRIVERTEX)) | : : | | | | : : | | | +->GRADIENT_FILL_RECT_H | | | | | GRADIENT_FILL_RECT_V +--------------+ | GRADIENT_FILL_TRIANGLE <---+ | | | TRIVERTEX | | | | v | [nVer-1] | / V V +--------------+ - -+--------------+ - -+--------------+ - - - - - - - - - - - ((char*)&Ver) + (nVer - 1) * sizeof(TRIVERTEX) | GRADIENT_RECT| | GRADIENT_TRI-| | [0] | | ANGLE[0] | +--------------+ +--------------+ | | | | : : : : | | | | +--------------+ +--------------+ | GRADIENT_RECT| | GRADIENT_TRI-| | [nTri-1] | | ANGLE[nTri-1]| +--------------+ +--------------+
There is a word transformation in the description which is apparently a typo.
The iType is said to be one of the values MWT_IDENTITY (1), MWT_LEFTMULTIPLY (2) or MWT_RIGHTMULTIPLY (3). However, in real metafiles, the numerical value 4 is found, which has no MWT_ symbol defined for it.
The description of the structure is nonsense. It is a feeble attempt by someone who didn't understand C to try to explain something using a C structure that cannot be explained using a C structure. It even states the PT_CLOSEFIGURE is added using the bitwise-XOR operator, which is gibberish.
The correct documentation should be completely rewritten as
typedef struct tagEMRPOLYDRAW {
EMR emr;
RECTL rclBounds;
DWORD cptl;
BYTE Data[1];
} EMRPOLYDRAW, *PEMRPOLYDRAW;
And state, as follows
Data[1]
An array of cptl POINTL structures, representing 32-bit data points expressed in logical units. Following this array, there will be an array cptl bytes, representing how each of the corresponding POINTL values should be interpreted. The array of bytes will be one of PT_MOVETO, PT_LINETO or PT_BEZIERTO, and for the values PT_LINETO and PT_BEZIERTO, the PT_CLOSEFIGURE flag could be ORed in using the bitwise-OR operator.
The actual structure, as currently implemented, is (ignoring the completely nonsensical abTypes field at the end of the structure)
+-------------+ | emr | | | +-------------+-----------+ | rclBounds | .left | | | .top | | | .right | | | .bottom | +-------------+-----------+ | cptl | +-------------+-----------------+ - - - - - - - - - - | aptl | aptl[0] | ^ +-------------+-----------------+ | | aplt[1] | | +-----------------+ | | | | cptl * sizeof(POINTL) : : | | | | +-----------------+ | | aplt[cptl-1] | v +-----------+-----+ - - - - - - - - - - ((char*)&aptl[0]) + cptl * sizeof(POINTL) | ty[0] | ^ +-----------+ | | | | : : | cptl * sizeof(BYTE) | | | +-----------+ | | ty[cptl-1]| v +-----------+ - - - - - - - - - - - - -
The description of the structure is nonsense. It is a feeble attempt by someone who didn't understand C to try to explain something use a C structure that cannot be explained using a C structure.
The correct documentation should be completely rewritten as
typedef struct tagEMRPOLYDRAW {
EMR emr;
RECTL rclBounds;
DWORD cptl;
BYTE Data[1];
} EMRPOLYDRAW, *PEMRPOLYDRAW;
And state, as follows
Data[1]
An array of cptl POINTS structures, representing 32-bit data points expressed in logical units. Following this array, there will be an array cptl bytes, representing how each of the corresponding POINTL values should be interpreted. The array of bytes will be one of PT_MOVETO, PT_LINETO or PT_BEZIERTO, and for the values PT_LINETO and PT_BEZIERTO, the PT_CLOSEFIGURE flag could be ORed in using the bitwise-OR operator.
The actual structure, as currently implemented, is (ignoring the completely nonsensical abTypes field at the end of the structure)
+-------------+ | emr | | | +-------------+-----------+ | rclBounds | .left | | | .top | | | .right | | | .bottom | +-------------+-----------+ | cpts | +-------------+-----------------+ - - - - - - - - - - | apts | apts[0] | ^ +-------------+-----------------+ | | apts[1] | | +-----------------+ | | | | cpts * sizeof(POINTS) : : | | | | +-----------------+ | | apts[cpts-1] | v +-----------+-----+ - - - - - - - - - - ((char*)&apts[0]) + cpts * sizeof(POINTS) | ty[0] | ^ +-----------+ | | | | : : | cpts * sizeof(BYTE) | | | +-----------+ | | ty[cpts-1]| v +-----------+ - - - - - - - - - - - - -
The structure is nonsensical. It isn't at all clear what is going on here. Since it is impossible to have variable-length arrays in structures, having two adjacent variable-length structures cannot happen. The document might explain it as I think it is intended:
aPolyCounts
This is the first element of an array of DWORD values that be the lpdwPolyPoints parameter to PolyPolyLine. Following this array is an array of POINTL values.
aptl
This is a "placeholder" and is not really a valid array element. The first element of the array would follow the aPolyCounts array
+----------------------+ | EMR emr | : : | | +----------------------+-------------+ | RECTL rclBounds | LONG left | | | LONG top | | | LONG right | | | LONG bottom | +----------------------+-------------+ | DWORD nPolys | +----------------------+ | DWORD cptl | +----------------------+----------------------+ - - - - - - - - - - - - - - - - - - | DWORD aPolyCounts[1] | aPolyCounts[0] | \ ^ +----------------------+----------------------+ | | | DWORD aptl[1] | aPolyCounts[1] | | | +----------------------+----------------------+ | | | aPolyCounts[2] | | | +----------------------+ > nPolys | nPolys * sizeof(DWORD) | | | | : : | | | | | | +----------------------+ | | | aPolyCounts[nPolys-1]| / V +----------------------+-----------------+ - - - - - - - - ((char*)&aPolyCounts[0]) + nPolys * sizeof(DWORD) | POINTL [0] | \ +-----------------+ | | POINTL [1] | | +-----------------+ | | | > cptl : : | | | | +-----------------+ | | POINTL [cptl-1] | / +-----------------+
See EMRPOLYPOLYLINE, EMRPOLYPOLYGON; the only change is the array of points is a POINTS array.
The documentation states the parameters, but they are incomplete, inaccurate, misleading and/or confusing.
dwFlags
Profile flags. [The meaning of this field is not documented, and there is no equivalent field in the SetICMProfile API]
cbName
Size of the desired profile name. [It is not stated if this is in characters or bytes, and for EMRSETICMPROFILEW, this is confusing. It also does not suggest if the string is NUL-terminated, and if it is, if this length includes the terminal NUL character]
cbData
Size of profile data, if attached [This is probably a correct description, but the nature of the profile data is unspecified and there is no hyperlink suggesting where the specification might be found]
Data[1]
Array size is cbName and cbData [This description doesn't even make sense. The correct description might be "This is the data; it represents the start of a variable-length array whose length is cbName + cbData, where the name, if cbName is non-zero, appears first." It might also state whether or not the cbName includes a terminating NUL character, and if there is a terminal NUL character present. In fact, could it be the equivalent of a MULTISZ sequence where each string ends with a NUL character but the entire sequence of strings ends with a sequece of two NUL character? Inquiring Minds Want To Know!]
This states that the ihObject is the index to a handle in the handle table, but neglects to mention that the ENHMETA_STOCK_OBJECT bit (0x80000000) bit can be present, in which case the low-order bits are interpreted as a stock object value from the selected values GetStockObject. Not only should it state this, but there should be a hyperlink to GetStockObject.
The ihObject field of this apparently can have the ENHMETA_STOCK_OBJECT bit ORed it to select a stock palette. See EMRSELECTOBJECT.
The offString and offDX parameters say they are the "offset to the string" and "offset to the spacing information" but this is incomplete. The correct documentation is
offString
The offset of the string, in bytes, measured from the beginning of the EMREXTTEXTOUTA or EMREXTTEXTOUTW structure. For EMREXTTEXTOUTA, this will point to a sequence of nChars 8-bit characters, and for EMREXTTEXTOUTW, this will point to a sequence of nChars Unicode characters. These characters are not NUL-terminated.
offDX
The offset of the intercharacter spacing array, measured from the beginning of the EMREXTTEXTOUTA or EMREXTTEXTOUTW structure. This points to an array of (nChars - 1) DWORD values which represent the intercharacter spacing. The values in this array will be DWORD-aligned.
The offDescription states that it is a pointer to "the array that contains the description of the enhanced metafile's contents". The correct description should be "Specifies the offset from the beginning of the ENHMETAHEADER structure to a string of Unicode characters that contains the description of the enhanced metafile contents"
While it states these are obsolete, what the documentation does not state is that these calls do appear in 32-bit code generated by earlier versions of 32-bit Visual Studio (in spite of the fact that the documentation says "Their functionality is incorporated into Microsoft's 32-bit operating systems". It is also worth pointing out that there is actual code in the VS6 versions (see file Program Files\Microsoft Visual Studio\vc98\mfc\src\app3d.cpp). So the fix for this is to add the additional conditional to the code below:
#if _MFC_VER < 0x0700 // Add this line #ifdef _AFXDLL Enable3dControls(); // Call this when using MFC in a shared DLL #else Enable3dControlsStatic(); // Call this when linking to MFC statically #endif #endif // _MFC_VER < 0x0700 // Add this line
See Enable3dControls.
Does not say what happens if a NULL pointer is passed in. Ideally,
void * p = EncodePointer(malloc(size));
should be NULL if malloc returns NULL.
Experimentation shows that it does not. This should be mentioned in the documentation.
This contains a number of problems. For example, it says it can "raise EXCEPTION_POSSIBLE_DEADLOCK" but it fails to point out that this is a RaiseException call and is not part of the C++ exception handling mechanism, which is not always obvious to most readers. It does not mention that EXCEPTION_POSSIBLE_DEADLOCK was not defined until VS2008, and in earlier versions of Windows it was defined as STATUS_POSSIBLE_DEADLOCK. The documentation, that it was available in earlier platforms, suggests that it was defined in earlier Platform SDKs for those platforms, and that is not true.
As a piece of user-friendly documentation, every change to a Platform SDK file or for that matter, any other library source file Microsoft distributes, should be logged in a comment at the start of the file, indicating the version level of the file and the changes that were made.
It says that under low memory situations, "EnterCriticalSection can raise an exception", but fails to say what exception is raised. It suggests using InitializeCriticalSectionAndSpinCount, but at no point does that API specify what the spin count is, what it means, or what the default spin count is assumed to be.
It specifies that the timeout interval for a CRITICAL_SECTION is specified in a Registry key, but it neglects to say what the units are.
There are interesting effects on the enumeration if the hwndParent is either HWND_MESSAGE or HWND_DESKTOP
Does not contain a cross-reference to the "=" directive (for redefinable values)
This linker directive, in the .def file, has confusing and misleading documentation. Some versions of the documentation suggest that a dense set of integers is required, which is not true.
From reader Jim Beveridge:
The EXPORTS section of a .def triggers some undocumented behavior that causes the generated LIB file to contain undecorated names. See my article on the subject (near the end – the “undecorated” attribute):
http://qualapps.blogspot.com/2007/08/how-to-create-32-bit-import-libraries.html
In the section labeled "Using extern to Specify Linkage", the example is stupidly misleading, and was clearly written by someone who understood nothing about how the C header files work. It suggests that you need to write
extern "C" {
#include <stdio.h>
}
which has to rank among the most incredibly stupid examples presented. This is because there is no reason to have to include stdio.h in the scope of an extern "C" declaration!
The crucial information that is required is not given, for example.
To create a header file for a C-based library that can be used for both C and C++, you need to put the extern "C" in conditionally, since it will not be recognized by the C compiler. The way to do this is to use the __cplusplus symbol to conditionally include the extern "C" only in C++ builds:
#ifdef __cpluspllus
extern "C" {
#endif
....your declarations here
#ifdef __cplusplus
} // extern "C"
#endifIt also does not mention that the notation
extern "C" {
declaration1;
declaration2;
}can be equally written as
extern "C" declaration1; extern "C" declaration2;
And therefore could be handled by writing
#ifdef __cplusplus #define MY_EXPORT extern "C" #else #define MY_EXPORT #endif
and therefore you can write in the header file
MY_EXPORT declaration(args);
and at the definition point
MY_EXPORT declaration(args)
{
... function body
}It concludes with the completely irresponsible and out-and-out erroneous example of declaring the variable errno as an extern int, which is nonsensical, because errno is usually not a variable (particularly, in the multithreaded C library), and it suggests that errno should be explicitly declared by the user ( a holdover from the early K&R C compilers; hello, has anyone been paying attention to the fact that we are not using K&R C any longer?), instead of doing #include <errno.h> which would be the correct way to handle it in the modern world. Anyone who learned to program after 1988 would know that you do not declare errno as an extern int and besides, in the multithreaded header file, it is declared as
_CRTIMP extern int * __cdecl _errno(void); #define errno (*_errno())
so the indicated declaration cannot possibly ever be right!
The examples of using trivial stdio examples by renaming the functions in trivial ways to illustrate the point is truly awful. There is also no rationale of why you would want to do this; the notion that this would be required by a static library or a DLL that is to be used by either C or C++ code is not actually mentioned.
Correct documentation would state that this is only required to access libraries that use C-style naming conventions; typically, this means a library (static or DLL) that has been built with the C compiler, but it should also mention that this is a technique to allow DLLs built using C++ to suppress the C++ "name mangling" and therefore simplify the use of GetProcAddress. It should further mention that this means that such function names cannot be overloaded using C++ language overload rules. GetProcAddress should contain a hyperlink to this page.
The documentation for this is
EXTERN [[langtype]] name [[(altid)]] : type [[, [[langtype]] name [[(altid)]] : type]] ...
Now, this would be a lot more useful if there was some way to know what permissible values could be used for langtype, what an altid was and when you would use it, and what values other than ABS were permitted for type. It also fails to mention that if ABS is a constant, it is a link-time constant, which is not at all the same as a compile-time constant.
Apparently, this documentation is only used inside Microsoft, where the reader can walk down the hall to the MASM maintainer, or read the source, to find out what is permitted here. Unfortunately, for the Rest Of Us, it isn't that easy.
Examination of the .cod output from the C/C++ compiler suggests that one of the permissible types is PROC. So we now have PROC and ABS. I wonder what else is permitted?
From a conversation on microsoft.public.vc.mfc. Question from "TheOne" and response by Mark Salsbery:
If the string pointer str is given as NULL and the nOptions contain ETO_OPAQUE, the operation just clears the rectangle specified by lpRect to the background color established by SetBkColor.
All occurrences of the word FAR in any definition, anywhere, are erroneous; this keyword died when Win32 was created, and its continued appearance confuses new users, who don't know that it is nonsense. The word should simply be dropped entirely from all usages, everywhere. It is still used extensively in the documentation and in the header files. See also PASCAL.
This example is excruciatingly poorly written. It only shows how to get a Unicode string, not a Unicode-aware string. The corrected example should be
#include <tchar.h> #define __TFILE__ _T(__FILE__) TCHAR * filename = __TFILE__;
This type is undocumented, although it is used in critical definitions, such as MAT2.
The type for an X86 is defined in wingdi.h:
typedef struct _FIXED {
short value;
WORD fract;
} FIXED;
The value part is a number in the range of -32768..32768 which represents the integer part of the range.
The fract part is a number in the rnage of 0..65535 which represents the interpolation between two values; it is accurate to one part in 65536. That is, "2.5" in floating point would be represented as <2, 32768>.
This MASM has the following documentation:
FPO (cdwLocals, cdwParams, cbProlog, cbRegs, fUseBP, cbFrame)
Parameters
Number of local variables, an unsigned 32 bit value.
Size of the parameters, an unsigned 16 bit value.
Number of bytes in the function prolog code, an unsigned 8 bit value.
Number of bytes in the function prolog code, an unsigned 8 bit value.
Indicates whether the EBP register has been allocated. either 0 or 1.
Indicates the frame type. See http://msdn.microsoft.com/en-us/library/ms679352.aspx for more information.
Notice that both the cbProlog and cbRegs have exactly the same documentation, yet there is no explanation as to why something designated as "regs" would represent bytes of prolog code. Also, fUseBP indicates "whether the EBP register has been allocated" but gives no explanation about what this means. Clearly, in an FPO function, the EBP is not used, so we are left to guess what might be mean by "allocated". Perhaps it means "Indicates if the contents of the EBP register can be modified by the suburoutine, thus rendering in meaningless", but who knows?
NOTE: DO NOT CONFUSE THIS WITH HeapFree, WHICH IS DIFFERENT!
The FreeHeap API describing what should be a typedef to a function. The problem is that its description is completely nonsensical in one critical area. It states
This function does not return a value. However, if the function sets Base to NULL, the buffer was freed. If Base is not NULL after the function call ends, the buffer could not be freed.
Note the definition
VOID FreeHeap(__int_out PVOID Base)
Now how could this possibly set Base to NULL? Only someone totally functionally illiterate in programming could possibly think that this description makes sense! It is IMPOSSIBLE for Base to be set to NULL!!!!
The correct definition is
The FreeHeap function is a member of the SECPKG_DLL_FUNCTIONS structure and specifies a pointer to the function to be called to deallocate memory previously allocated by the function specified by the AllocateHeap member of the SECPKG_DLL_FUNCTIONS structure
typedef VOID (NTAPI LSA_FREE_LSA_HEAP) (IN PVOID Base);
Note that because the function calling convention is NTAPI, ordinary C functions like malloc or new could not be specified.
Note that the absolutely essential calling convention type is not specified!
See also AllocateHeap
This code contains a serious bug, such that in a "text" mode file (that is, fopen(filename, "a") and other variants that include "a" in the mode), if the file was created on a Unix system, the code behaves incorrectly.
The error is in the way it computes the file position. It erroneously assumes that every \n must be preceded by a \r, and consequently has a loop of the form (found in the ...\crt\src\ftell.c file):
for (p = stream->_base; p < max; p++)
if (*p == '\n')
/* adjust for '\r' */
rdcnt++;
incorrectly adjusts when the file is a Unix-format file that does not have a \r before each \n. The correct code would look for the actual \r character instead of \n, or, alternatively, the test should see if each \n has a \r preceding it before incremented the rdcnt variable. As the code stands, it will not work correctly for a Unix-formatted file. Of course, it is likely that if this bug is reported it will be designated as "this is by design", which is a weasly way to say "we screwed it up, and because we screwed it up, we declare the screwup to be the correct design" (Q: How many Microsoft support people does it take to change a light bulb? A: None. They declare that the darkness is "by design")
These preprocessor symbols do not specify what version of Visual Studio they are defined in (they are not, for example, defined in VS6, but are in VS.NET 2003), and more importantly, do not say which value of _MSC_VER for which they are defined. Furthermore, macros do not "return" values, they are "defined as" values! Good documentation would say something like the following
| __FUNCDNAME__ | Valid only within a function. A quoted string which is the decorated name of the enclosing function (as an 8-bit character string; see below to get a Unicode-aware string). Defined only for _MSC_VER >= 1300. Not expanded if the /EP or /P compiler options are specified. This generates an 8-bit quoted string. If a Unicode string is required or a Unicode-aware string is required, you must use a level of indirection as shown below #include<tchar.h> #define __TFUNCDNAME__ _T(__FUNCDNAME__) or it could be written as shown below, which is a trace statement indicating entry to a function which takes an integer parameter TRACE(_T("=>") _T(__FUNCDNAME__) _T("(%d)\n"), parameter);
An example using 8-bit characters is class Test {
public:
void Show() {
printf("__FUNCTION__ [%s]\n", __FUNCTION__);
printf("__FUNCDNAME__ [%s]\n", __FUNCDNAME__);
printf("__FUNCSIG__ [%s]\n", __FUNCSIG__);
}
};
produces __FUNCTION__ [Test::Show] __FUNCDNAME__ [?Show@Test@@QAEXXZ] __FUNCSIG__ [void __thiscall Test::Show(void)] |
| __FUNCSIG__ | Valid only within a function. A quoted string which is the signature of the enclosing function (as an 8-bit character string; see below to get a Unicode-aware string). Defined only for _MSC_VER >= 1300/ Not expanded if the /EP or /P compiler options are specified. This generates an 8-bit quoted string. If a Unicode string is required or a Unicode-aware string is required, you must use a level of indirection as shown below #include<tchar.h> #define __TFUNCSIG__ _T(__FUNCSIG__) or it could be written as shown below, which is a trace statement indicating entry to a function which takes an integer parameter TRACE(_T("=>") _T(__FUNCSIG__) _T("(%d)\n"), parameter);
An example using 8-bit characters is class Test {
public:
void Show() {
printf("__FUNCTION__ [%s]\n", __FUNCTION__);
printf("__FUNCDNAME__ [%s]\n", __FUNCDNAME__);
printf("__FUNCSIG__ [%s]\n", __FUNCSIG__);
}
};
produces __FUNCTION__ [Test::Show] __FUNCDNAME__ [?Show@Test@@QAEXXZ] __FUNCSIG__ [void __thiscall Test::Show(void)] |
| __FUNCTION__ | Valid only within a function. A quoted string which is the undecorated name of the enclosing function (as an 8-bit character string; see below to get a Unicode-aware string). Defined only for _MSC_VER >= 1300. Not expanded if the /EP or /P compiler options are specified. This generates an 8-bit quoted string. If a Unicode string is required. or a Unicode-aware string is required, you must use a level of indirection as shown below #include <tchar.h> #define __TFUNCTION__ _T(__FUNCTION__) or it could be written as shown below, which is a trace statement indicating entry to a function which takes an integer parameter TRACE(_T("=>") _T(__FUNCTION__) _T("(%d)\n"), parameter);
An example using 8-bit characters is class Test {
public:
void Show() {
printf("__FUNCTION__ [%s]\n", __FUNCTION__);
printf("__FUNCDNAME__ [%s]\n", __FUNCDNAME__);
printf("__FUNCSIG__ [%s]\n", __FUNCSIG__);
}
};
produces __FUNCTION__ [Test::Show] __FUNCDNAME__ [?Show@Test@@QAEXXZ] __FUNCSIG__ [void __thiscall Test::Show(void)] |
This is poorly documented.
| lStructSize | Specifies the size, in bytes, of the structure. [no change here] |
| lpOutString | Pointer to a buffer that receives the output string. This may be NULL if the output string is not needed. This buffer must be at least the value of nCount+1 where nCount is the parameter to GetCharacterPlacement The output string is a version of the original string, in the order that the characters will be displayed on the specified device associated with the HDC. Typically, the output string is identical to the original string, but may be different if the string needs reordering and the GCP_REORDER flag is set, or if the original string exceeds the maximum extent and GCP_MAXEXTENT flag is set. The output string will be NUL-terminated, and therefore the buffer size allocated must be equal to the number of characters in the input string, plus one. |
| lpOrder | A pointer to the array that receives the ordering indices, or NULL if the ordering indices are not needed. The length of the array should be set to the number of characters in the nCount parameter of GetCharacterPlacement. The meaning of the order array depends on other elements of GCP_RESULTS
The typical usage of this would be to obtain the ordering if GetFontLanguageInfo returns the GCP_REORDER flag, which indicates that the original string requires reordering for display. |
| lpDX | An array of int values. The length of the array is the number of characters in the nCount parameter for GetCharacterPlacement. This pointer may be NULL if these distances are not needed. If glyph rendering is done, the distances are for the glyphs, and not the characters, so the resulting array in this case can be used with the ExtTextOut function. The distances in this array appear in display order. To find the distance for the ith character in the original string, the lpOrder array can be used as follows: width = lpDx[lpOrder[i]]; |
| lpCaretPos | Pointer to the array that receives the caret position values. The length of this array should be at least as long as the number of characters specified by the nCount parameter for GetCharacterPlacement. This member may be NULL if the caret positions are not needed. Each value specifies the caret position immediately before the corresponding character. In some languages the position of the caret for each character may not be immediately to the left of the character; for Hebrew, a right-to-left-reading language, the before position is to the right of the character. If glyph ordering is done, lpCaretPos matches the original input string, not the output string. In this case, some adjacent values may be the same (if, for example, there is a mix of right-to-left and left-to-right text). The values in the array are in input order. To find the caret position value for the ith character in the original string, use the array as follows position = lpCaretPos[i]; |
| lpClass | Pointer to an array that contains and/or receives character classifications. The length of this array should be at least as long as the number of characters specified by the nCount parameter for GetCharacterPlacement. If the GCP_CLASSIN flag is specified for GetCharacterPlacement, this array must be initialized to the character class flags desired. [rest of text] |
| lpGlyphs | Pointer to an array that receives the values identifying glyphs used for rendering of the string. Its length should be as long as the number of characters specified by the nCount parameter for GetCharacterPlacement. It may be NULL if glyph rendering is not needed. This parameter should be set to NULL if glyph rendering is not needed. Note that the number of glyphs in the array may be less than the number of characters in the original string if the string contains ligated glyphs; the actual count of glyphs will be found in the nGlyphs member, which must be initialized to the maximum length. Also, if reordering is required, the order of the glyphs may not be sequential. This array is used if more than one operation is being done n a string which any form of ligation, kerning, or order-switching. This array allows for operations to use the same array and not have to compute it each time. This array always contains glyph indices. When using ExtTextOut to produced kerned output, the ETO_GLYPH_INDEX flag must be specified in the ExtTextOut call. |
| nGlyphs | [as in documentation] |
GDI coordinates are limited to 32 bits. What is undocumented in Win64 is that they seem to be limited to bitmaps whose physical size is < 2GB, so if you have a 24-bit RGB bitmap, some operations (such as StretchBlt) will fail.
The documentation of the GDI+ flat API is an example of documentation arrogance at its worst. There are no detailed discussions of any of the methods or their parameters, and most especially their return values, or the reason that an error might occur. You might think, as the idiots who decided not to document these methods, that "nobody needs to know that because they are using the wrapper classes" but in fact when the wrapper classes fail in some way, and you single-step into them, you find they are calling these low-level functions with parameters you cannot understand, and you cannot discover what they should be, because they are undocumented. So you cannot figure out what is missing. An example is the GdipCreateFromHBITMAP call, which requires a palette, but in fact the primitive for creating the Bitmap object that is used inside CImage::Save does not seem to provide for a means to create a palette from an 8-bit bitmap. Essentially, the quality of this documentation is inexcusable. Exactly what part of "documentation" was missed by the creators of this trash?
The list of options is very incomplete. In addition it lists
APPCOMMAND_MEDIA_SELECT
an option which does not otherwise exist.
In GM_ADVANCED mode, the ABC widths returned by GetCharABCWidths are different from the widths returned by that API when GM_COMPATIBLE mode is used.
Note that the ABC widths are dynamically adjusted based on font size (and do not scale linearly with font magnification) and may also depend upon the mapping mode selected. The same character in the same font may not give the same ABC widths in MM_TEXT and MM_ISOTROPIC even though everything else in the DC is the same.
While it is true that this is obsolete, its documentation is still poor.
For example, GCP_RESULTS is not hyperlinked except at the top of the page. Every instance that is reasonable should be hyperlinked.
The documentation for GCP_CLASSIN syas "Specifies that the lpClass array contains preset classifications..". It should say "Specifies that the lpClass member of the GCP_RESULTS structure contains preset classifications...". In addition the reference to "see GCP_RESULTS is not hyperlinked.
There is no example of how this could be used to take advantage of pair kerning.
It does not mention that if the GCP_USEKERNING flag is set, the lpGlyphs member of the GCP_RESULTS must be non-NULL and must be at least as long as the nCount parameter, or the API will return with a failure and GetLastError will return error 87, ERROR_INVALID_PARAMETER. This should be explicitly stated with the GCP_USEKERNING flag.
This states that if the text is not in the correct form for the request, it will be implicitly converted; related documentation states that if the data is, for example, in CF_UNICODETEXT, and the program is an ANSI program that is asking for CF_TEXT, then it will be converted. However, it does not state what code page is used for the conversion. Presumably the active code page as obtained from GetACP( ), but this is not stated and should be.
It appears to be undocumented; correspondent Kero points out that the result of GetClassName or an HWND_MESSAGE window is _T("Message").
TCHAR name[MAX_PATH];
if(GetClassName(hwnd, name, MAX_PATH))
{
ASSERT(_tcscmp(name, _T("Message")) == 0);
...
}
In a later note, he points out that in the case of common controls, CommCtrl.dll version 6 (themes), and reports that
GetClassName == RealGetWindowClass
in that the string returned by GetClassName is the same string returned by RealGetWindowClass.
This led me to investigate the definitions of these two functions.
The GetClassName function retrieves the name of the class to which the specified window belongs
int GetClassName(HWND nWnd, LPTSTR lpClassName, int nMaxCount);
or
The RealGetWindowClass function retrieves a string that specifies the window type.
UINT RealGetWindowClass(HWND hWnd, LPTSTR pszType, UINT cchType);
Now, other than the change from int to UINT, I don't see any difference between these two calls. What is the distinction between a "window type" and a "class"? In fact, given the name of the API, why does it not say that it "retrieves a name of the window class to which the specified window belongs"? This leaves me wondering what is the difference between these two calls? Note that neither of these APIs cross-references the other.
This also returns the string for a Registered Window Message.
(thanks to Giovanni Dicanio for this tip!)
For some properties of some devices, the GetDeviceCaps API returns -1. For example, it can return -1 for NUMBRUSHES, NUMPENS, or NUMCOLORS. There is no documentation that suggests what this can mean; it merely says "Number of device-specific brushes". There is nothing in the Return Value section to suggest that there might be an illegal value returned, or what the meaning of a negative number would be.
This is not indexed in the VS2008 documentation.
The Parameters description is incomplete:
lpFilePart
[out] Pointer to a buffer that receives the address (in lpBuffer) of the file name component in the path. If this value is not needed, this parameter can be NULL.
This fails to have hyperlinks to ImageCodecInfo and GetImageEncoderSize at all instances of these terms. This is caused by people who are stupid enough to believe English majors who have written that a hyperlink should appear only once per page. Such an attitude is childish, and pointless, because it forces me to search the page to find where the one-and-only hyperlink is, instead of letting me use it as an intelligent design would, by clicking the word at the point where it appears! PLEASE DO NOT BELIEVE THE KINDS OF IDIOTS WHO WRITE THESE RULES! THEY DO NOT ACTUALLY EVER USE THE PAGES CREATED ACCORDING TO THESE ASININE PRINCIPLES! THEREFORE, THEY ARE UNQUALIFIED TO CREATE SUCH RULES, AND ANYONE WHO BELIEVES THEM IS ACTING IRRESPONSIBLY!
In addition, the ImageCodecInfo hyperlink does not appear in the See Also section!
The example code that accompanies this is another example of the failure of anyone to actually supervise the amateurs who write these examples. It contains the horrific code
BOOL rc;
rc = Glpi(buffer, &returnLength);
if(FALSE == rc)
{
...stuff
}
Comparing a Boolean variable to a Boolean literal shows a total lack of understanding about what a Boolean variable actually means. It is stupid and irresponsible to allow code like this to appear as example code.
The simple solution, since the return value is not used anywhere, is to write
if(!Glpi(buffer, &returnLegnth))
{
...stuff
}
There is no reason to introduce a gratuitous variable for this purpose, and there is no excuse for comparing a Boolean value to a Boolean literal to get, guess what, a Boolean value!
Similarly, the introduction of the gratuitous done variable. Why introduce a gratuitous variable when the break statement does everything required? Sloppy, sloppy coding, based on some academic's silly rule about the use of break, no doubt.
But what is truly sad is that the processor information block is of static size and therefore no loop is required to repeat the buffer sizing.
DWORD len;
PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer;
if(!GetLogicalProcessorInformation(NULL, &len))
{ /* failed (expected) */
DWORD err = ::GetLastError();
if(err != ERRROR_INSUFFICIENT_BUFFER)
{ /* actual failure */
...deal with failure
} /* actual failure */
buffer = (PSYSTEM_LOGICAL_PROCESSOR_INFORMATION)malloc(len);
if(!GetLogicalProcessorInformation(buffer, &len))
{ /* total failure */
...deal with failure
} /* total failure */
This is not actually an API, but is a really-badly-written piece of code from the Web page http://msdn.microsoft.com/en-us/library/aa446670(VS.85).aspx titled "Getting the Logon SID in C++". What we do not get is an example of getting the logon SID in C++; what we have is a badly-written piece of pure C code. Like most of the other MSDN examples that claim to be in C++, this was written by some beginner who had never seen C++, had no idea what the language was, had never learned how to program properly in C, and did not receive any adult supervision to say "this piece of code is complete trash, rewrite it so it looks like it was written by an intelligent human being". As with other examples, I will present the code side-by-side with a decent example.
As usual, it contains a lot of goto statements, proving beyond any shadow of a doubt that the person knows nothing about how to program correctly. And instead of using new, it uses HeapAlloc. Why anyone in C++ would use HeapAlloc escapes me. Then it declares all the variables, including loop counters, at the top of the block. Hello? What part of "C++" was not comprehended by the programmer?
| Original code | Rewritten code |
| Header file: LogonSid.h | |
|
Note that API calls should be preceded by "::" to indicate they are in the global namespace. Instead of using primitive C structures like pointers, a useful C++ class is defined. Since I normally use MFC, I would have used CString for the ToString method instead of the rather obsolete std::string. If you support Unicode, you need to have the lines in a header file somewhere: #ifdef _UNICODE typedef tstring std::string; #else typedef tstring std::wstring; #endif and replace the std::string declarations shown below with tstring. It is unfortunate that std:: does not have built-in the concept of Unicode-aware strings. |
|
| Apparently there is no header file used for this subroutine example |
class CSID {
public:
//****************
// CSID()
// !CSID()
//****************
CSID() { }
virtual ~CSID() { }
//****************************************************************
// Get
// Result: PSID
// A pointer to the SID if one is present
// NULL if there is no SID
//****************************************************************
PSID Get() { return buffer.size() > 0 ? (PSID)&buffer[0] : NULL; }
//****************************************************************
// CopySid(PSID sid)
// Inputs:
// PSID sid: The SID to be copied into this structure
// Result: BOOL
// TRUE if successful
// FALSE if error
//****************************************************************
BOOL CopySid(PSID sid) {
DWORD len = ::GetLengthSid(sid);
buffer.resize(len);
if(!::CopySid(len, (PSID)&buffer[0], sid))
{ /* failed */
buffer.resize(0);
return FALSE;
} /* failed */
return TRUE;
}
//****************************************************************
// ToString
// Result: std::tstring
// The SID expressed as a string
// This will be the empty string if there is an error
//****************************************************************
std::string ToString() {
LPTSTR result;
if(::ConvertSidToStringSid((PSID)&buffer[0], &result))
{ /* success */
std::string t = result;
LocalFree(result);
return t;
} /* success */
else
return std::string(_T(""));
}
//****************
protected:
std::vector
|
| File LogonSid.cpp | |
#include <windows.h> |
#include <stdafx.h> #include <LogonSid.h> |
| It is not at all clear why all the variables are declared at the top of the function. The title clearly says that this example is written in C++. It is not clear why a temporary structure that is used only inside a single function and which must be deleted before exiting is declared as if the code had been written for the PDP-11 in 1975. In C++, the programmer, had he or she been even marginally literate, would have used at std::vector<BYTE>. | |
BOOL GetLogonSID (HANDLE hToken, PSID *ppsid)
{
BOOL bSuccess = FALSE;
DWORD dwIndex;
DWORD dwLength = 0;
PTOKEN_GROUPS ptg = NULL;
|
BOOL GetLogonSid(HANDLE hToken, CSID & sid)
{
std::vector<BYTE> tg;
// Get required buffer size and allocate the TOKEN_GROUPS buffer.
DWORD dwLength = 0;
|
// Verify the parameter passed in is not NULL.
if (NULL == ppsid)
goto Cleanup;
|
Only a C programmer would pass in a PSID *. A C++ programmer would have used a PSID &. I chose to implement a class CSID that provides useful functionality. Since I use a reference, there is no need to check the parameter to see if it is NULL. And the goto Cleanup is simply childish. There is no reason to go to a cleanup routine when there is nothing to clean up! |
| In the code below, there is no reason to pass the argument ptg into the call; NULL is sufficient. There is no reason to cast it to LPVOID because that is implicitly handled by the nature of the C++ language. Note that if GetTokenInformation would succeed, the world is really screwed up. There is no need to use the childish goto or to goto Cleanup at all, because, surprise, there is nothing to clean up! To allocate the required buffer space, I simply set the size of the std::vector to be the size I want. Now there is never a need to goto Cleanup to get rid of it because the destructor of std::vector will free the space! |
|
// Get required buffer size and allocate the TOKEN_GROUPS buffer.
if (!GetTokenInformation(
hToken, // handle to the access token
TokenGroups, // get information about the token's groups
(LPVOID) ptg, // pointer to TOKEN_GROUPS buffer
0, // size of buffer
&dwLength // receives required buffer size
))
{
if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
goto Cleanup;
ptg = (PTOKEN_GROUPS)HeapAlloc(GetProcessHeap(),
HEAP_ZERO_MEMORY, dwLength);
if (ptg == NULL)
goto Cleanup;
}
|
if (!::GetTokenInformation(
hToken, // handle to the access token
TokenGroups, // get information about the token's groups
NULL, // buffer pointer (ignored)
0, // size of buffer
&dwLength // receives required buffer size
))
{ /* allocate buffer */
if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
return FALSE;
tg.resize(dwLength);
} /* allocate buffer */
else
{ /* nominally impossible */
return FALSE;
} /* nominally impossible */
|
| In the code below, I obtain a pointer to the buffer. If the operation fails, I can simply return FALSE and don't need the childish goto. In pure C, this could be handled with _try/_finally or simply by allocating in a function outside, and calling a second function that produced the result; the calling function will always delete any dynamically-allocated storage when the function it calls returns. You can always spot a programmer too lazy to write a second function because of how they use the goto. But this code is allegedly C++, and it should be written in C++. | |
// Get the token group information from the access token.
if (!GetTokenInformation(
hToken, // handle to the access token
TokenGroups, // get information about the token's groups
(LPVOID) ptg, // pointer to TOKEN_GROUPS buffer
dwLength, // size of buffer
&dwLength // receives required buffer size
))
{
goto Cleanup;
}
|
// Get the token group information from the access token.
if (!::GetTokenInformation(
hToken, // handle to the access token
TokenGroups, // get information about the token's groups
(LPVOID) &tg[0], // pointer to TOKEN_GROUPS buffer
dwLength, // size of buffer
&dwLength // receives required buffer size
))
{
return FALSE;
}
|
| In the original code, the DWORD dwIndex was declared at the top of the block, as if it were 1975 PDP-11 code. This, however, is C++. A variable should not be declared until it is needed, and in the case of a loop index, if it is not needed outside the loop construct, it should be declared within the for-loop itself. Since I'm actually writing in C++, instead of PDP-11 C, I actually use genuine C++ constructs. There seemed to be no reason to use a pointer which might be NULL when I could declare a reference parameter, and there seemed to be no reason to use primitive allocation (such as the silly HeapAlloc calls) when working in C++, so I put all the smarts, including how to do a copy, in the class. I sort of took the idea of writing the code in C++ seriously as opposed to being noise words. |
|
// Loop through the groups to find the logon SID.
for (dwIndex = 0; dwIndex < ptg->GroupCount; dwIndex++)
if ((ptg->Groups[dwIndex].Attributes & SE_GROUP_LOGON_ID)
== SE_GROUP_LOGON_ID)
{
// Found the logon SID; make a copy of it.
dwLength = GetLengthSid(ptg->Groups[dwIndex].Sid);
*ppsid = (PSID) HeapAlloc(GetProcessHeap(),
HEAP_ZERO_MEMORY, dwLength);
if (*ppsid == NULL)
goto Cleanup;
if (!CopySid(dwLength, *ppsid, ptg->Groups[dwIndex].Sid))
{
HeapFree(GetProcessHeap(), 0, (LPVOID)*ppsid);
goto Cleanup;
}
break;
}
bSuccess = TRUE;
Cleanup:
// Free the buffer for the token groups.
if (ptg != NULL)
HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
return bSuccess;
}
|
// Loop through the groups to find the logon SID.
PTOKEN_GROUPS ptg = (PTOKEN_GROUPS)&tg[0];
BOOL result = FALSE;
for (DWORD dwIndex = 0; dwIndex < ptg->GroupCount; dwIndex++)
{ /* scan tokens */
if ((ptg->Groups[dwIndex].Attributes & SE_GROUP_LOGON_ID) == SE_GROUP_LOGON_ID)
{ /* logon SID */
// Found the logon SID; make a copy of it.
if(!sid.CopySid(ptg->Groups[dwIndex].Sid))
return FALSE;
result = TRUE;
break;
} /* logon SID */
} /* scan tokens */
return result;
}
|
| There are serious questions here about the competence of the programmer who wrote this example. Such as: why is an explicit deallocation required in C++? Why is this apparently written as a separate .cpp file? Why is it using HeapFree instead of delete? The code in the box below illustrates the total amount of code that is required to accomplish this task when it is written competently in C++. | |
#include <windows.h>
VOID FreeLogonSID (PSID *ppsid)
{
HeapFree(GetProcessHeap(), 0, (LPVOID)*ppsid);
}
|
|
|
34 lines of poorly-structured 1975-style C code 01 lines of missing header file === 35 lines of incredibly-poorly-written code reminiscent of bad programming in 1975 C |
19 lines of well-structured C++ 18 lines of a generally-useful C++ class ==== 37 lines of well-structured C++ consistent with the concept of "Getting the Logon SID in C++" |
The example which follows this description is nonsensical. The return type is BOOL, and therefore it should be treated as a BOOL value. Saying that a BOOL value can be -1 makes no sense whatsoever. If the return type is int, it should be declared as int and the description should read that the int value can be -1 (error), 0 (quit) or >0 (successful retrieval of a message). It should not involve declaring a BOOL variable and treating it as if it were an int.
The documentation of this API is inadequate. It points out that the filename can be in the "\\?\" format, but it is not obvious to the unsophisticated reader what the implication of this is. It isn't helped by the fact that the API does not actually give a failure, merely silently truncates the file name; failing to check for ERROR_INSUFFICIENT_BUFFER means that the file name may not be correct.
The documentation stats that the global variable _pgmptr is automatically initialized to the full path, but it omits rather useful information, such as the type of the variable. Is in an LPTSTR? Inquiring Minds Want To Know.
While in most cases, simply using MAX_PATH (and the number of times I have erroneously seen the value "256" substituted for MAX_PATH is amazing), the fully-general solution is not at all obvious. For example, the classic repeat-until-smaller loop will not occur to most programmers.
It should be pointed out that
TCHAR name[MAX_PATH]; GetModuleFileName(NULL, name, MAX_PATH);
will work most of the time, the only fully-correct algorithm is the one shown below
LPTSTR p;
DWORD len = MAX_PATH;
while(TRUE)
{ /* get name */
p = (LPTSTR)malloc(len * sizeof(TCHAR));
DWORD n = GetModuleFileName(NULL, p, len);
if(n == 0)
{ /* error */
...deal with error
break; // for example
} /* error */
if(n < len)
break; // success
len *= 2;
free(p);
} /* get name */
or in MFC, (and note that the existence of MFC must not be ignored!)
CString s;
DWORD len = MAX_PATH;
while(TRUE)
{ /* get name */
p = s.GetBuffer(len);
DWORD n = ::GetModuleFileName(NULL, p, len);
s.ReleaseBuffer();
if(n == 0)
{ /* error */
... deal with error
break; // for example
} /* error */
if(n < len)
break; // success
len *= 2;
} /* get name */
The example shown in the article "Allocating Memory NUMA Node [Base]" shows a slovenly approach to coding that under no conditions should ever be encouraged. The example contains the test
if(TRUE != GetNumaProcessorNode(i, &NodeNumber))
This is stupid and irresponsible. The specification of GetNumaProcessorNode clearly states,
"If the function succeeds, the value is nonzero"
At no point does it state that this value is going to be TRUE. It is the height of slovenliness to ever compare a Boolean value to the literal TRUE or FALSE because the value is itself a Boolean value. It is unfortunate that the people who code these examples have neither adult supervision nor do they bother to actually read the documentation of the functions they are trying to show how to use! It is also unfortunate they were never trained as programmers.
This states "Retrieves the node number for the specified proximity identifier".
It does not give the slightest hint as to what a "proximity identifier" actually is!
There are no symbols defined in the header file.
Prior to VS2005, this description contains a gross misstatement of fact: it says "a process affinity mask is a proper subset of a system affinity mask". This statement is simply stupid. It shows that the author had no idea what "proper subset" means. People who do not know how to use terms should not try to be pretentious by using them. A "proper subset" means that the the subset cannot equal the set itself, and it is perfectly clear that a process affinity mask can equal the system affinity mask! Even the revised documentation, which drops the word "proper", can be confusing to those who think that "subset" must mean "proper subset". The correct documentation should say
A process affinity mask may not specify a 1 bit where the system affinity mask specifies a 0 bit. A process affinity cannot exceed the number of processors in the system.
The documentation states that the parameters to the API as
BOOL GetProcessWorkingSetSize(HANDLE hProcess, PSIZE_T lpMinimumWorkingSetSize, PSIZE_T lpMaximumWorkingSetSize);
however, the example which follows is erroneous; it declares the variables as type DWORD. This will not port to Win64. The example was not corrected when the documentation was updated for Win64.
This does not tell the resolution of the values returned. It only says the units are 100ns units of FILETIME; it does not suggest what the resolution of the times actually is. A bit of experimentation suggests that the resolution is the resolution of the system timer, 15ms on a multiprocessor.
The example titled "Getting the login SID in C++" (http://msdn.microsoft.com/en-us/library/aa446670(VS.85).aspx) is a particularly good example of especially bad coding style. There is very little "right" about it. It is not only abysmal C++, it is actually very bad C programming as well. There is no excuse for code this poor to be presented as an example!
(GetTokenInformation)BOOL GetLogonSID (HANDLE hToken, PSID *ppsid)
*************************************************************************************************
and what part of "C++" did the author of this code miss? The sensible thing to do here is to designate the second parameter as a PSID & psid. Only a C programmer would think a pointer made sense here!
*************************************************************************************************
{
BOOL bSuccess = FALSE;
*************************************************************************************************
The only reason this variable exists is because the code is badly written; it does not need to exist
*************************************************************************************************
DWORD dwIndex;
***************************