A Slipshod Check of the Visual C++ 2013 Library (update 3)

• Analysis results
  • Incorrect index check
  • Incorrect format
  • Strange repeating calculations
  • Typo suspected
  • Genuine typo
  • Executing same actions regardless of condition
  • Incorrect port number check
  • Virtual destructor missing
  • Incomplete code
  • Suspicious double initialization
  • Suspicious status check
  • Incorrect argument for the MAKE_HRESULT macro
  • ASSERT's with always true conditions
  • Superfluous type conversions
  • Using pointers before a check
  • Superfluous variables
• Miscellaneous
  • Dangerous comparisons to TRUE
  • Increment
  • Incorrect warning status restoration
  • The check (this == NULL)
• Conclusion

Someone suggested to me recently that I check the libraries from Visual Studio 2013. I haven't found anything of much interest, just a few small errors and slip-ups. They wouldn't make an interesting, attractive article, but I've still decided to describe all those defects. I just hope it will help make the libraries a bit better and stimulate the authors to carry out a more thorough analysis. I don't have the project files necessary to build the libraries, so my analysis had to be superficial and I could have missed a lot.

This is the second article about analysis of the Visual C++ libraries. For results of the previous check, see the article Errors detected in the Visual C++ 2012 libraries.

I cannot analyze the libraries in full and what I did was a pretty slipshod check: I included into a new project all the files from the folders "crt\src" and "atlmfc\src" and also created a new test.cpp file to include all the header files related to the standard library (vector, map, set, etc.) in.

After that, I played around with the project settings a bit and finally managed to get about 80% of files to compile. I think that's quite enough. Even if a file cannot compile, PVS-Studio usually can check it anyway, even if only partly.

I think if the libraries' developers find this article useful and interesting, they will carry out a more thorough analysis. Even the exotic building process is no longer a problem as you can use the compiler monitoring system.

I used PVS-Studio 5.19 to do the analysis. I checked the source codes of the C/C++ libraries included into Visual Studio 2013 (update 3).

Analysis results

I've found a few defects which were found in the previous version, Visual Studio 2012, too. For instance, the proj() function is still implemented in a pretty strange manner; the ~single_link_registry() destructor is written in the same dangerous way. But it's not interesting to tell the same story. Let's try to find something new.

Incorrect index check

void _Initialize_order_node(...., size_t _Index, ....)
{
  if (_Index < 0)
  {
    throw std::invalid_argument("_Index");
  }
  ....
}

PVS-Studio's diagnostic message: V547 Expression '_Index < 0' is always false. Unsigned type value is never < 0. agents.h 8442

The '_Index' argument is unsigned. That's why the check doesn't make any sense because no exception will ever be generated. It looks like superfluous code rather than an error.

Incorrect format

int _tfpecode; /* float point exception code */

void __cdecl _print_tiddata1 (
  _ptiddata ptd
)
{
  ....
  printf("\t_gmtimebuf      = %p\n",   ptd->_gmtimebuf);
  printf("\t_initaddr       = %p\n",   ptd->_initaddr);
  printf("\t_initarg        = %p\n",   ptd->_initarg);
  printf("\t_pxcptacttab    = %p\n",   ptd->_pxcptacttab);
  printf("\t_tpxcptinfoptrs = %p\n",   ptd->_tpxcptinfoptrs);
  printf("\t_tfpecode       = %p\n\n", ptd->_tfpecode);
  ....
}

PVS-Studio's diagnostic message: V576 Incorrect format. Consider checking the second actual argument of the 'printf' function. The pointer is expected as an argument. tidprint.c 133

What we're dealing here with is the last line effect. There is a mistake at the end of a block of similarly looking lines. In each line, a pointer value should be printed, but in the last line, the '_tfpecode' variable is just an integer value, not a pointer. What should have been written instead is the following:

printf("\t_tfpecode       = %i\n\n", ptd->_tfpecode);

Strange repeating calculations

unsigned int SchedulerProxy::AdjustAllocationIncrease(....) const
{
  ....
  unsigned int remainingConcurrency = 
                         m_maxConcurrency - m_currentConcurrency;
  remainingConcurrency = m_maxConcurrency - m_currentConcurrency;
  ....
}

PVS-Studio's diagnostic message: V519 The 'remainingConcurrency' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1136, 1137. schedulerproxy.cpp 1137

The variable is assigned the result of one and the same expression twice. This code is superfluous and most likely resulted from poor refactoring.

Typo suspected

double HillClimbing::CalculateThroughputSlope(....)
{
  ....
  MeasuredHistory * lastHistory = GetHistory(fromSetting);
  MeasuredHistory * currentHistory = GetHistory(toSetting);
  ....
  double varianceOfcurrentHistory = currentHistory->VarianceMean();
  double varianceOflastHistory = currentHistory->VarianceMean();
  ....
}

PVS-Studio's diagnostic message: V656 Variables 'varianceOfcurrentHistory', 'varianceOflastHistory' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'currentHistory->VarianceMean()' expression. Check lines: 412, 413. hillclimbing.cpp 413

It is suspicious that the variables varianceOfcurrentHistory and varianceOflastHistory are assigned one and the same value. It would be more logical to initialize the varianceOflastHistory variable in the following way:

double varianceOflastHistory = varianceOfcurrentHistory;

Moreover, there is also the 'lastHistory' pointer. My supposition is that there is a typo in this code and it was most likely meant to look like this:

double varianceOfcurrentHistory = currentHistory->VarianceMean();
double varianceOflastHistory = lastHistory->VarianceMean();

Genuine typo

BOOL CPropertySheet::SetActivePage(CPropertyPage* pPage)
{
  ASSERT_VALID(this);
  ENSURE_VALID(pPage);
  ASSERT_KINDOF(CPropertyPage, pPage);

  int nPage = GetPageIndex(pPage);
  ASSERT(pPage >= 0);

  return SetActivePage(nPage);
}

PVS-Studio's diagnostic message: V503 This is a nonsensical comparison: pointer >= 0. dlgprop.cpp 1206

It is strange to check a pointer value for being larger than or equal to zero. This is obviously a typo and the programmer actually wanted to check the 'nPage' variable:

int nPage = GetPageIndex(pPage);
ASSERT(nPage >= 0);

That's just an ASSERT of course and the error won't cause any serious troubles, but it's still an error.

Executing same actions regardless of condition

void CMFCVisualManager::OnDrawTasksGroupCaption(....)
{
  ....
  if (pGroup->m_bIsSpecial)
  {
    if (!pGroup->m_bIsCollapsed)
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
                        rectButton.TopLeft());
    }
    else
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
                        rectButton.TopLeft());
    }
  }
  else
  {
    if (!pGroup->m_bIsCollapsed)
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
                        rectButton.TopLeft());
    }
    else
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
                        rectButton.TopLeft());
    }
  }
  ....
}

PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanager.cpp 2118

Regardless of the (pGroup->m_bIsSpecial) condition, the same actions are executed. That's strange.

Incorrect port number check

typedef WORD ATL_URL_PORT;
ATL_URL_PORT m_nPortNumber;

inline BOOL Parse(_In_z_ LPCTSTR lpszUrl)
{
  ....
  m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf);
  if (m_nPortNumber < 0)
    goto error;
  ....
}

PVS-Studio's diagnostic message: V547 Expression 'm_nPortNumber < 0' is always false. Unsigned type value is never < 0. atlutil.h 2773

The 'm_nPortNumber' variable has the unsigned WORD type.

Virtual destructor missing

class CDataSourceControl
{
  ....
  ~CDataSourceControl();
  ....
  virtual IUnknown* GetCursor();
  virtual void BindProp(....);
  virtual void BindProp(....);
  ....
}

CDataSourceControl* m_pDataSourceControl;

COleControlSite::~COleControlSite()
{
  ....
  delete m_pDataSourceControl;
  ....
}

PVS-Studio's diagnostic message: V599 The destructor was not declared as a virtual one, although the 'CDataSourceControl' class contains virtual functions. occsite.cpp 77

The CDataSourceControl class contains virtual methods but the destructor is not virtual. That's dangerous: if an X class is inherited from the CDataSourceControl class, you won't be able to destroy objects of the X type using a pointer to the base class.

Incomplete code

BOOL CMFCWindowsManagerDialog::OnHelpInfo(HELPINFO* pHelpInfo)
{
  pHelpInfo->iCtrlId;
  CWnd* pParentFrame = AfxGetMainWnd();
  pParentFrame->SendMessage(AFX_WM_WINDOW_HELP, 0,
                            (LPARAM) this);
  return FALSE;
}

PVS-Studio's diagnostic message: V607 Ownerless expression 'pHelpInfo->iCtrlId'. afxwindowsmanagerdialog.cpp 472

What is "pHelpInfo->iCtrlId;"? What does it mean?

Suspicious double initialization

CMFCStatusBar::CMFCStatusBar()
{
  m_hFont = NULL;

  // setup correct margins
  m_cxRightBorder = m_cxDefaultGap;  // <=
  m_cxSizeBox = 0;

  m_cxLeftBorder = 4;
  m_cyTopBorder = 2;
  m_cyBottomBorder = 0;
  m_cxRightBorder = 0;               // <=
  ....
}

PVS-Studio's diagnostic message: V519 The 'm_cxRightBorder' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 74, 80. afxstatusbar.cpp 80

At first, a value of another variable is written into the 'm_cxRightBorder' variable. And then it is suddenly set to zero.

Suspicious status check

#define S_OK  ((HRESULT)0L)
#define E_NOINTERFACE  _HRESULT_TYPEDEF_(0x80004002L)

HRESULT GetDocument(IHTMLDocument2** ppDoc) const
{  
  const T* pT = static_cast<const T*>(this);
  return pT->GetDHtmlDocument(ppDoc) ? S_OK : E_NOINTERFACE;
}

HRESULT GetEvent(IHTMLEventObj **ppEventObj) const
{
  ....
  if (GetDocument(&sphtmlDoc))
  ....
}

PVS-Studio's diagnostic message: V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'GetDocument(& sphtmlDoc)'. The SUCCEEDED or FAILED macro should be used instead. afxhtml.h 593

The code formatting does not seem to meet the code execution logic. What you may think at first is that if the 'GetDocument(...)' condition is true, you've managed to get the document. But really it's all quite otherwise. The GetDocument() function returns a value of the HRESULT type. And it's all different about this type. For example, the S_OK status is coded as 0 and the E_NOINTERFACE status as 0x80004002L. To check values of the HRESULT type, special macros should be used: SUCCEEDED, FAILED.

I don't know for sure if there is an error here, but still this code is confusing and needs to be checked.

Incorrect argument for the MAKE_HRESULT macro

#define MAKE_HRESULT(sev,fac,code) \
  ((HRESULT) \
   (((unsigned long)(sev)<<31) | \
    ((unsigned long)(fac)<<16) | \
    ((unsigned long)(code))) )

ATLINLINE ATLAPI AtlSetErrorInfo(....)
{
  ....
  hRes = MAKE_HRESULT(3, FACILITY_ITF, nID);
  ....
}

PVS-Studio's diagnostic message: V673 The '(unsigned long)(3) << 31' expression evaluates to 6442450944. 33 bits are required to store the value, but the expression evaluates to the 'unsigned' type which can only hold '32' bits. atlcom.h 6650

The code will work as it should but there is still an error inside it. Read on for the explanation.

The function must form an error message inside a variable of the HRESULT type. For this purpose, the MAKE_HRESULT macro is used. Yet it is used incorrectly. The programmer assumed that the first parameter 'severity' takes values from within the range between 0 and 3. He must have confused it with the way in which error codes are formed when working with the functions GetLastError()/SetLastError().

The MAKE_HRESULT macro can take only 0 (success) or 1 (failure) as the first argument. For details on this subject, see the forum at the CodeGuru site: Warning! MAKE_HRESULT macro doesn't work.

Since the number 3 is used as the first actual argument, an overflow occurs. The number 3 will "turn" into 1. This lucky accident prevents the error from affecting program execution.

ASSERT's with always true conditions

There are quite a lot of fragments where an ASSERT condition is implemented in the (X >= 0) pattern. At the same time, an X variable is declared as the unsigned integer type. So the condition turns out to be always true.

In some cases, the use of ASSERT's like that is valid - e.g. when a variable may become signed due to refactoring and the algorithm is not ready to handle negative numbers. In this code, however, using some of those doesn't seem to make any sense. They should be removed from the code or replaced with other useful checks. That's why I decided to mention them in the article.

Check this example:

DWORD m_oversubscribeCount; 

void ExternalContextBase::Oversubscribe(....)
{
  if (beginOversubscription)
  {
    ASSERT(m_oversubscribeCount >= 0);
    ++m_oversubscribeCount;
  }
  ....
}

PVS-Studio's diagnostic message: V547 Expression 'm_oversubscribeCount >= 0' is always true. Unsigned type value is always >= 0. externalcontextbase.cpp 204

And here is the list of all the other issues of this kind:

• V547 Expression 'm_oversubscribeCount >= 0' is always true. Unsigned type value is always >= 0. internalcontextbase.cpp 506
• V547 Expression 'pGlobalNode->m_idleCores >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 3764
• V547 Expression 'pGlobalNode->m_availableCores >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 3769
• V547 Expression 'pReceivingProxyData->m_allocation >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4100
• V547 Expression 'pReceivingProxyData->m_allocation >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4360
• V547 Expression 'exclusiveCoresAvailable >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4657
• V547 Expression 'coresNeeded >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4657
• V547 Expression 'previousGlobal >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4781
• V547 Expression 'currentGlobal >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4782
• V547 Expression 'm_minConcurrency >= 0' is always true. Unsigned type value is always >= 0. schedulerproxy.cpp 63
• V547 Expression 'm_minimumHardwareThreads >= 0' is always true. Unsigned type value is always >= 0. schedulerproxy.cpp 125
• V547 Expression 'm_oversubscribeCount >= 0' is always true. Unsigned type value is always >= 0. umsthreadinternalcontext.cpp 308
• V547 Expression 'j >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 1922
• V547 Expression 'pMaxNode->m_availableCores >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 2542
• V547 Expression 'previousLocal >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4793
• V547 Expression 'currentLocal >= 0' is always true. Unsigned type value is always >= 0. resourcemanager.cpp 4794
• V547 Expression is always true. Unsigned type value is always >= 0. schedulerpolicybase.cpp 285
• V547 Expression 'value >= 0' is always true. Unsigned type value is always >= 0. schedulerpolicybase.cpp 345

Superfluous type conversions

I've found a few explicit type conversions which are not just superfluous but may also spoil values.

Example one:

size_t __cdecl strnlen(const char *str, size_t maxsize);
size_t __cdecl _mbstrnlen_l(const char *s,
                            size_t sizeInBytes,
                            _locale_t plocinfo)
{
  ....
  if ( _loc_update.GetLocaleT()->locinfo->mb_cur_max == 1 )
      /* handle single byte character sets */
      return (int)strnlen(s, sizeInBytes);
  ....
}

PVS-Studio's diagnostic message: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'strnlen(s, sizeInBytes)'. _mbslen_s.c 67

The strnlen() function returns a value of the 'size_t' type. Then it is suddenly explicitly cast to the 'int' type. After that, the value will be implicitly extended back to the size_t type.

This code contains a potential 64-bit issue. Should one try in a 64-bit program version to calculate the number of characters in a very long string using the _mbstrnlen_l() function, one will get an incorrect result.

I guess this explicit type conversion was left in the code by accident and just needs to be removed.

Example two:

WINBASEAPI SIZE_T WINAPI GlobalSize (_In_ HGLOBAL hMem);

inline void __cdecl memcpy_s(
  _Out_writes_bytes_to_(_S1max,_N)  void *_S1,
  _In_ size_t _S1max,
  _In_reads_bytes_(_N) const void *_S2,
  _In_ size_t _N);

AFX_STATIC HGLOBAL AFXAPI _AfxCopyGlobalMemory(....)
{
  ULONG_PTR nSize = ::GlobalSize(hSource);
  ....
  Checked::memcpy_s(lpDest, (ULONG)::GlobalSize(hDest),
                    lpSource, (ULONG)nSize);
  ....
}

PVS-Studio's diagnostic message: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'nSize'. olemisc.cpp 684.

The GlobalSize() function returns the SIZE_T type. The memcpy_s() function's arguments have the size_t type too.

Then what is the "(ULONG)::GlobalSize(hDest)" explicit type conversion for?

If we start working with a buffer larger than 4 Gb, the memcpy_s() function will only copy a part of the array.

There are a few other suspicious type conversions:

• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen(* vp ++)'. cenvarg.c 236
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: '::GlobalSize(m_hGlobalMemory)'. fileshrd.cpp 48
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen(lpsz)'. dumpcont.cpp 31
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen(lpsz)'. dumpcont.cpp 82
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: '(cElems * sizeof (CLSID))'. ctlcore.cpp 1975
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wParam'. afxtoolbarslistcheckbox.cpp 94
• V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'nChars * sizeof (TCHAR)'. statreg.h 270

Using pointers before a check

CMFCPopupMenu* CMFCCustomizeButton::CreatePopupMenu()
{
  ....
  if (m_pWndParentToolbar->IsLocked())
  {
    pMenu->GetMenuBar()->m_pRelatedToolbar = m_pWndParentToolbar;
  }

  pMenu->m_bRightAlign = m_bMenuRightAlign &&
    (m_pWndParentToolbar->GetExStyle() & WS_EX_LAYOUTRTL) == 0;

  BOOL bIsLocked = (m_pWndParentToolbar == NULL ||
                    m_pWndParentToolbar->IsLocked());
  ....
}

PVS-Studio's diagnostic message: V595 The 'm_pWndParentToolbar' pointer was utilized before it was verified against nullptr. Check lines: 192, 199. afxcustomizebutton.cpp 192

The 'm_pWndParentToolbar' pointer is first dereferenced in the 'm_pWndParentToolbar->IsLocked()' expression and then is checked for being null: 'm_pWndParentToolbar == NULL'.

It is dangerous code and I don't think I should explain why.

Another case like that:

void COleControlSite::BindDefaultProperty(....)
{
  ....
  if (pDSCWnd != NULL)
  {
    ....
    m_pDSCSite = pDSCWnd->m_pCtrlSite;
    ....
    m_pDSCSite->m_pDataSourceControl->BindProp(this, TRUE);
    if (m_pDSCSite != NULL)
      m_pDSCSite->m_pDataSourceControl->BindColumns();
  }
  ....
}

PVS-Studio's diagnostic message: V595 The 'm_pDSCSite' pointer was utilized before it was verified against nullptr. Check lines: 1528, 1529. occsite.cpp 1528

Superfluous variables

Superfluous variables are not errors. But since they are superfluous, one still don't want them in code and should get rid of them. For example:

int GetImageCount() const
{
  CRect rectImage(m_Params.m_rectImage);
  if (m_Bitmap.GetCount() == 1)
  {
    HBITMAP hBmp = m_Bitmap.GetImageWell();
    BITMAP bmp;

    if (::GetObject(hBmp, sizeof(BITMAP), &bmp) ==
        sizeof(BITMAP))
    {
      return bmp.bmHeight / m_Params.m_rectImage.Height();
    }

    return 0;
  }

  return m_Bitmap.GetCount();
}

PVS-Studio's diagnostic message: V808 'rectImage' object of 'CRect' type was created but was not utilized. afxcontrolrenderer.h 89

The 'rectImage' rectangle is created but is not used in any way after that. Thus, we've got one extra line in the program and a few extra processor clock cycles to run when working with the Debug version.

Here is a file with a list of all the superfluous variables: vs2003_V808.txt

Miscellaneous

Quite a lot of warnings from PVS-Studio point out poor coding style rather than errors. My opinion is that the source codes of the Visual C++ libraries should serve as a role model for other programmers and it's no good teaching them bad things.

Some fragments that can be improved are cited below.

Dangerous comparisons to TRUE

_PHNDLR __cdecl signal(int signum, _PHNDLR sigact)
{
  ....
  if ( SetConsoleCtrlHandler(ctrlevent_capture, TRUE)
       == TRUE )
  ....
}

PVS-Studio's diagnostic message: V676 It is incorrect to compare the variable of BOOL type with TRUE. winsig.c 255

Every source, including MSDN, tells us that it's a bad practice to compare anything to TRUE. The function may return any value other than 0 and that will count as TRUE. But TRUE is 1. So the correct way of calculating such a comparison is Foo() != FALSE.

Other similar comparisons:

• V676 It is incorrect to compare the variable of BOOL type with TRUE. event.cpp 448
• V676 It is incorrect to compare the variable of BOOL type with TRUE. Correct expression is: 'retVal != FALSE'. resourcemanager.cpp 1437
• V676 It is incorrect to compare the variable of BOOL type with TRUE. Correct expression is: 'retVal != FALSE'. resourcemanager.cpp 5027

Increment

void _To_array(
  ::Concurrency::details::_Dynamic_array<_EType>& _Array)
{
  _LockHolder _Lock(_M_lock);
  _M_iteratorCount++;

  for(_LinkRegistry::iterator _Link = _M_links.begin();
      *_Link != NULL; _Link++)
  {
    _Array._Push_back(*_Link);
  }
}

PVS-Studio's diagnostic message: V803 Decreased performance. In case '_Link' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. agents.h 1713

It's just such a subtle nuance, but all the sources recommend using ++iterator. Wherever possible, it is better to use a prefix operator as a good coding style for others to learn.

Note. A few posts on the subject:

• Is it reasonable to use the prefix increment operator ++it instead of postfix operator it++ for iterators?.
• Pre vs. post increment operator - benchmark.

If the libraries' authors decide they should work on those increments, here is the list of all the fragments I've found: vs2003_V803.txt.

Incorrect warning status restoration

#pragma warning (disable : 4311)
SetClassLongPtr(m_hWnd,
  GCLP_HBRBACKGROUND,
  PtrToLong(reinterpret_cast<void*>(
    ::GetSysColorBrush(COLOR_BTNFACE))));
#pragma warning (default : 4311)

The V665 diagnostic message: Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 165, 167. afxbasepane.cpp 167

A correct way to restore the previous warning status is to use "#pragma warning(push[ ,n ])" and "#pragma warning(pop)".

Other similar fragments: vs2003_V665.txt.

The check (this == NULL)

That's a classic of the genre:

_AFXWIN_INLINE CWnd::operator HWND() const
  { return this == NULL ? NULL : m_hWnd; }

PVS-Studio's diagnostic message: V704 'this == 0' expression should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin2.inl 19

Unfortunately, that's a very common pattern - especially in MFC. But programmers should gradually learn to give up using constructs like that and set a good example to others instead.

Those who don't yet know why it is bad, see the documentation on the V704 diagnostic for a detailed explanation.

I understand that the operator HWND() really can't be fixed: the backward compatibility is more important. But why not do that wherever it can be done without painful consequences? Here is the list of all the checks of this kind: vs2003_V704.txt

Conclusion

As you can see, the article turns out to be pretty large. But actually there's nothing too interesting or crucial found in the libraries; their code is definitely of a high quality and well debugged.

I will be glad if this article helps make the Visual C++ libraries a bit better in the future. Let me point it out once again that what I've done was an incomplete analysis. The Visual C++ libraries' developers can carry out a much better and more thorough one as they have scripts/projects to build the libraries. If you face any troubles, I'll be glad to help you solve them - contact our support service.