C 编程的 42 条建议（三）

刀首木 2017-03-17

展开全文

21. 检查文件终止符（EOF）有正确到达终点

让我们继续讨论和文件有关的话题，再来看EOF。但这次我们要讨论的是完全不同的bug类型。它通常只出现在本地化软件版本里。

下面的代码片段是选自Computational Network Toolkit。错误被PVS-Studio诊断为：V739 EOF不应该和 char 类型比较。‘c’应该是int类型。

string fgetstring(FILE* f)
{
  string res;
  for (;;)
{
    char c = (char) fgetc(f);
    if (c == EOF)
     RuntimeError('error reading .... 0: %s', strerror(errno));
    if (c == 0)
       break;
       res.push_back(c);
  }
   return res;
}

解释

让我们来看EOF是怎样声明的：

#define EOF (-1)

如你所见，EOF 就是一个值为‘-1’的 int 类型。Fgetc()返回一个int类型的值。也就是说，它可以返回 0 到 255 的一个数，或者是-1（EOF）。

使用扩展字符集（Extended ASCII Codes）的用户在程序中某一个字母处理不当的时候可能会出错。

比如说，在 Windows1251 编码表中，俄文的最后一个字母是 0xFF ，而在程序中就会被编译为文件终止符。

正确代码

for (;;)
{
int c = fgetc(f);
if (c == EOF)
RuntimeError('error reading .... 0: %s', strerror(errno));
if (c == 0)
break;
res.push_back(static_cast(c));
}

建议

在这里并没有什么特殊的建议，但既然我们谈到了 EOF，我想展示一些人没有注意到的比较有趣的错误。

只要记住，如果一个函数的返回值是 int 类型，就不要急于把它转化为 char 类型。停下来检查看有没有错。顺便提一句，我们在第二个技巧“大于 0 并不意味着是1 ” 那里讨论 memcmp（）函数的时候用到了相似的代码。（关于 MySQL 漏洞那一块）。

22. 不要用 #pragma warning(default:X)

下面的代码来自 TortoiseGIT 项目。该错误被 PVS-Studio 诊断为：V665 也许，在此处，‘#pragma warning(default: X)'用得不对。应该用 '#pragma warning(push/pop)'

#pragma warning(disable:4996)
LONG result = regKey.QueryValue(buf, _T(''), &buf_size);
#pragma warning(default:4996)

解释

程序员经常认为，早期让警告实现的'pragma warning(disable: X)'指令在使用'pragma warning(default : X)'后就能继续工作。但并非如此。'pragma warning(default : X)'这条指令是把‘X’ 警告设置为 DEFAULT 状态，这两个不是同一件事。

假设一个文件在编译时用了-wall，那么就会出现 C4061 警告。如果你增加了'#pragma warning(default : 4061)' 指令，那么这条警告就不会出现，因为它已经被默认关掉了。

正确代码

#pragma warning(push)
#pragma warning(disable:4996)
LONG result = regKey.QueryValue(buf, _T(''), &buf_size)
#pragma warning(pop)

建议

返回警告的之前状态的正确方法是使用'#pragma warning(push[ ,n ])' 和'#pragma warning(pop)'这两条指令。可以看Visual C++11关于这些指令描述的文档：: Pragma Directives. Warnings.

库开发者应该要特别注意V665警告。忽视定制警告会在库使用者这边引起巨大的灾难。

关于这个话题的，值得一看的好文章：So, You Want to Suppress This Warning in Visual C++

23. 自动计算字符串长度

下面的代码来自OpenSSL库。错误被PVS-Studio诊断为：V666 检查‘strncmp’的第三个参数。它可能和字符串的长度不一致，字符串长度取决于第二个参数。

if (!strncmp(vstart, 'ASCII', 5))
arg->format = ASN1_GEN_FORMAT_ASCII;
else if (!strncmp(vstart, 'UTF8', 4))
arg->format = ASN1_GEN_FORMAT_UTF8;
else if (!strncmp(vstart, 'HEX', 3))
  arg->format = ASN1_GEN_FORMAT_HEX;
else if (!strncmp(vstart, 'BITLIST', 3))
  arg->format = ASN1_GEN_FORMAT_BITLIST;
else
....

解释

很难停止使用魔数。而且没有理由不使用类似0, 1, -1, 10这些常数。更难的是，给这些常数命名，而且，它们会使阅读代码变得更复杂。

然而，减少使用魔数的个数是有用的。比如说，避免使用用来定义字符串长度的魔数就很有用。

让我们来看一下上面给出的代码。代码看上去很像是复制粘贴的。程序员复制了下面这一行：

else if (!strncmp(vstart, 'HEX', 3))

之后，用'BITLIST'代替'HEX' ，但是程序员忘了把3改为7. 结果就是，字符串不是和'BITLIST'做比较，而是仅仅比较了'BIT'。这个错误似乎不太严重，但终究是个错误。

用复制粘贴来写代码真的很糟糕。更严重的是，字符串长度由魔数来定义。一直以来，我们遇到这样的错误，就是字符串长度和确定的数字不一致的错误，都是因为拼写错误或者程序员的疏忽。所以，这是一个典型的错误，我们需要做点什么来避免它。让我们来看看应该如何避免。

正确代码

乍一看，只要把 strncmp() 换成 strcmp() 就好了。那样魔数就消失了。

else if (!strcmp(vstart, 'HEX'))

不好——我们改变了代码运作的逻辑。strncmp() 的作用是检查一个字符串是否是以‘HEX’开始的，而 strcmp() 是检查两个字符串是否相等的。它们做的是不同的检查。

要解决这个问题最简单的方法就是改变常数：

else if (!strncmp(vstart, 'BITLIST', 7)) arg->format = ASN1_GEN_FORMAT_BITLIST;

这个代码是正确的，但是魔数 7 还在那里，这就有点不好了。这也是为什么我要建议另一种方法。

建议

如果我们能够正确估计字符串的长度，这样的错误就能够避免。最简单的方法就是用 strlen() 函数。

else if (!strncmp(vstart, 'BITLIST', strlen('BITLIST')))

在下面例子中，如果你忘记更改字符串，你能更快地发现它们的不匹配。

else if (!strncmp(vstart, 'BITLIST', strlen('HEX')))

但是这个建议有两个缺点：

无法保证编译器会不会优化 strlen() 调用：用一个常数来代替它。
你要逐字复制字符串。看上去不好看，而且也会出错。

第一个问题可以在编译阶段用专门计算字符串长度的结构来解决。比如说，你可以用这样的宏：

#define StrLiteralLen(arg) ((sizeof(arg) / sizeof(arg[0])) - 1) ....
else if (!strncmp(vstart, 'BITLIST', StrLiteralLen('BITLIST')))

但这个宏有点危险。在重构的时候会出现下面的代码：

const char *StringA = 'BITLIST';
if (!strncmp(vstart, StringA, StrLiteralLen(StringA)))

在这个例子中，StrLiteralLen 宏会返回一些没意义的东西。取决于指针的大小（4 或 8 字节），我们会得到 3 或 7 。但是在 C++ 中我们可以避免这种尴尬的局面，通过使用更复杂的技巧：

template
char (&ArraySizeHelper(T (&array)[N]))[N];
#define StrLiteralLen(str) (sizeof(ArraySizeHelper(str)) - 1)

现在，如果 StrLiteralLen 宏的参数是一个简单的指针，我们可能无法编译上面的代码。

让我们来看第二个问难题（复制字符串）。我不知道要对 C 程序员说什么。你可以为它写一个特别的宏，但是我个人不喜欢这样。我不热衷于宏。所以我知道要建议什么。

在 C++ 中，一切都很好。而且，我们可以用更聪明的法子来解决第一个问题。模板函数会帮我们很大的忙。你可以用不同的方法写，但是一般是长这样的：

template
int mystrncmp(const T *a, const T (&b)[N])
{
return _tcsnccmp(a, b, N - 1);
}

现在，字符串只用一次。在编译阶段就能计算字符串的长度。你不会遇到简单指针也不会错误的计算字符串的长度。完美！

总结：在处理字符串的时候避免使用魔数。使用宏或模板函数。这样函数不仅会变得更安全，而且会更漂亮更简短。

作为例子，你可以看 strcpy_s () 的声明：

errno_t strcpy_s(
  char *strDestination,
  size_t numberOfElements,
  const char *strSource
  );
template
errno_t strcpy_s(
  char (&strDestination)[size],
  const char *strSource
); // C++ only

第一个是针对于 C 语言的，或者在不是提前知道缓冲区大小的情况的。如果我们要处理缓冲区，创建一个栈，这样我们就可以在 C++ 中用第二个了。

24. Override 和 final 标识符应该成为你的新朋友

下面的代码片段选自 MFC 库。该错误被 PVS-Studio 诊断为：V301 意外的函数重载行为。观察函数 'WinHelpW' 的第一个参数，这个函数在派生类'CFrameWndEx' 和基类 'CWnd' 中。

class CWnd : public CCmdTarget {
  ....
  virtual void WinHelp(DWORD_PTR dwData,
                       UINT nCmd = HELP_CONTEXT);
  ....
};
class CFrameWnd : public CWnd {
  ....
};
class CFrameWndEx : public CFrameWnd {
  ....
  virtual void WinHelp(DWORD dwData,
                       UINT nCmd = HELP_CONTEXT);
  ....
};

解释

当你重写一个虚函数的时候，很容易在函数签名的时候出错，也很容易变成定义一个新的函数，这个函数跟基类里的虚函数没有任何关联。在这个例子中，有这几类错误：

在被重写的函数中，参数类型不同。
在被重写的函数中，参数个数不同。在参数比较多的时候，这种情况会更严重。
重写的函数在 const 修饰符这里不同。
基类函数不是虚函数。这种情况是以为在派生类中的函数会重载基类中的函数，但事实上，并没有，派生类隐藏了它。

相同的错误也会在修改已存在的代码中的参数类型或参数个数的时候发生。当程序员改变要改变虚函数签名的时候，他可能改了几乎所有有继承关系的类里的，但忘了在一些派生类中改，于是，就出错了。

在将代码移植到64位平台的时候，这种错误出现得更平凡。因为这个时候要把DWORD 改成 DWORD_PTR , LONG , LONG_PTR 等等。这就是我们这个例子中发生的错误。

即使有这种错误的代码能够在 32 位系统正常运行，在 64 位机子上也会出错。因为在 32 位上，DWORD 和 DWORD_PTR 是 unsigned long 的同义词，但是在 64 位平台上，DWORD_PTR 是 unsigned __int64 的同义词。

正确代码

class CFrameWndEx : public CFrameWnd {
  ....
  virtual void WinHelp(DWORD_PTR dwData,
                       UINT nCmd = HELP_CONTEXT) override;
  ....
};

现在有方法来避免上面描述的错误了。在 C++11 里添加了两个标识符：

Override——表明该函数是重写基类中的虚函数。
Final——表明该函数在派生类中无需重写。

我们对 override 这个标识符比较感兴趣。这个标识符可以指示编译器去检查某个虚函数是不是重写基类里的函数，如果不是，就报错。

如果在写 CFrameWndEx 类里的 WinHelp 函数的时候我们有用 override 指明，这个应用的 64 位版本在编译的时候会出错。所以，这个错误应该在早期清除掉。

在重写虚函数的时候记得用 override（或final）标识符。更多关于 override 和 final 的知识请看下面的文章：

Cppreference.com. override specifier (since C++11)
Cppreference.com. final specifier (since C++11)
维基百科. Explicit overrides and final.
Stackoverflow.com. 'override' in c++11.

25. 不要再拿‘this’和 nullptr 比较了

下面的代码来自 CoreCLR 项目。这段危险的代码被 PVS-Studio 诊断为：V704 应避免 'this == nullptr' 这个表达式——这个表达式在新的编译器上总是 false，因为‘this’ 指针永远不会为 NULL。

bool FieldSeqNode::IsFirstElemFieldSeq()
{
  if (this == nullptr)
    return false;
  return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}

解释

人们以前常拿 this 指针来和 0/ NULL / nullptr 来做比较。这种情况只有在 C++ 开发阶段的早期是正常的。我们是在‘考古学’研究的时候发现这样的代码的。我建议阅读一篇关于checking Cfront 的文章了解更多关于它们的细节。而且，在以前，this指针的值是可以改变的，但过去太久了，这件事已经被遗忘。

让我们再回过头来看 this 和 nullptr 之间的比较。

现在它是非法的。根据现代 C++ 标准，this 永远不会等于 nullptr。

根据 C++ 标准，调用 IsFirstElemFieldSeq() 方法来访问空指针 this 会导致未定义行为。

看上去它是想表示，如果 this==0，则在运行这个函数的时候就不能进入该区域。但事实上，这段代码有两种完全不同的实现方式。根据 C++ 标准，this指针用于不会为空，所以编译器会通过简化它来优化函数调用：

bool FieldSeqNode::IsFirstElemFieldSeq()
{
return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}

对了，这里还有一个陷阱。假设这里有一个多重继承：

class X: public Y, public FieldSeqNode { .... };
....
X * nullX = NULL;
X->IsFirstElemFieldSeq();

假设 Y 类的大小是 8 字节。然后源指针 NULL（0x00000000）在这样的情况下是对的，然后它又指向了下一个对象 FieldSeqNode 的开始处，然后你要偏移sizeof（Y）字节。所以指向 IsFirstElemFieldSeq() 函数的 this 指针应该是0x00000008。最后，检查'this == 0'也没什么意义了。