[clang-tidy] Add cppcoreguidelines-avoid-do-while check

[carlosgalvezp]

Summary

Implements rule ES.75 of C++ Core Guidelines.

[carlosgalvezp]

Fix typo

[carlosgalvezp]

Document default value

[carlosgalvezp]

Add check to list of checks.

[carlosgalvezp]

Fix typo

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst

7

Please synchronize first statement with Release Notes. This check us usually omitted.

22

Links usually placed at the end.

42

Please use single back-ticks for options values.

[carlosgalvezp]

Address comments

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst

22

I think having the link after the options would make it a bit disconnected. Instead I typically find Options at the very end, after the description.

See for example:
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/owning-memory.html

WDYT?

[carlosgalvezp]

PS: @aaron.ballman I didn't add you as reviewer since you have expressed in the past that you'd prefer not to review C++ Core Guidelines patches. Let me know if that has changed :)

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst

22

Also:
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/no-malloc.html

[carlosgalvezp]

Add a couple more tests.

[njames93]

The AllowWhileFlase option seems the wrong way to go about silencing do while(false) macros. Would it not make more sense to just ignore macros, or if you want more specificty don't warn on macros with a false condition?

[carlosgalvezp]

In D132461#3748812, @njames93 wrote:

The AllowWhileFlase option seems the wrong way to go about silencing do while(false) macros. Would it not make more sense to just ignore macros, or if you want more specificty don't warn on macros with a false condition?

Strictly speaking, the C++ Core Guidelines do not make any such exception for macros. That's why I thought an option would allow both users who want to comply strictly, and users who want a bit more flexibility (without having to NOLINT their way out of it). Similar options exist for other C++ Core Guidelines checks.

[carlosgalvezp]

In D132461#3748818, @carlosgalvezp wrote:

In D132461#3748812, @njames93 wrote:

The AllowWhileFlase option seems the wrong way to go about silencing do while(false) macros. Would it not make more sense to just ignore macros, or if you want more specificty don't warn on macros with a false condition?

Strictly speaking, the C++ Core Guidelines do not make any such exception for macros. That's why I thought an option would allow both users who want to comply strictly, and users who want a bit more flexibility (without having to NOLINT their way out of it). Similar options exist for other C++ Core Guidelines checks.

Or do you mean renaming the AllowWhileFalse option to IgnoreMacros and make the exception broader? I'd be fine with that as well, I'm not particularly happy with the AllowWhileFalse name :)

[carlosgalvezp]

Rename option to IgnoreMacros and update logic and tests.

[carlosgalvezp]

@njames93 Is there anything else I should address?

[carlosgalvezp]

@njames93 Friendly ping. The patch has addressed all comments and remained unchanged for 2 weeks. I would like to land it latest next week. If you are happy with the patch, please accept the review. Alternatively, please let me know if you intend to continue reviewing.

[carlosgalvezp]

Adjust warning message for consistency.

[njames93]

In D132461#3773792, @carlosgalvezp wrote:

@njames93 Friendly ping. The patch has addressed all comments and remained unchanged for 2 weeks. I would like to land it latest next week. If you are happy with the patch, please accept the review. Alternatively, please let me know if you intend to continue reviewing.

I do intend, but I'm currently between 2 jobs and accommodation atm, once life settles down next week, I should have more time.

[carlosgalvezp]

In D132461#3793634, @njames93 wrote:

In D132461#3773792, @carlosgalvezp wrote:

@njames93 Friendly ping. The patch has addressed all comments and remained unchanged for 2 weeks. I would like to land it latest next week. If you are happy with the patch, please accept the review. Alternatively, please let me know if you intend to continue reviewing.

I do intend, but I'm currently between 2 jobs and accommodation atm, once life settles down next week, I should have more time.

Totally understandable, thanks for the response! Are there other reviewers that could review the patch instead, to remove some load off you?

[carlosgalvezp]

@njames93 Ping

[bigdavedev]

LGTM. All comments addressed and the check world as intended.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h

+//===--- AvoidDoWhileCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// do-while loops are less readable than plan while loops, and can lead to
+/// subtle bugs.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-do-while.html
+class AvoidDoWhileCheck : public ClangTidyCheck {
+public:
+  AvoidDoWhileCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  bool IgnoreMacros;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp

+//===--- AvoidDoWhileCheck.cpp - clang-tidy -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidDoWhileCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+AvoidDoWhileCheck::AvoidDoWhileCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", false)) {}
+
+void AvoidDoWhileCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+}
+
+void AvoidDoWhileCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(doStmt().bind("x"), this);
+}
+
+void AvoidDoWhileCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<DoStmt>("x")) {
+    if (IgnoreMacros && MatchedDecl->getBeginLoc().isMacroID())
+      return;
+    diag(MatchedDecl->getBeginLoc(), "avoid do-while loops");
+  }
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidConstOrRefDataMembersCheck.cpp
+  AvoidDoWhileCheck.cpp
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidConstOrRefDataMembersCheck.h"
+#include "AvoidDoWhileCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"

 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidConstOrRefDataMembersCheck>(
         "cppcoreguidelines-avoid-const-or-ref-data-members");
+    CheckFactories.registerCheck<AvoidDoWhileCheck>(
+        "cppcoreguidelines-avoid-do-while");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-suspicious-realloc-usage
   Finds usages of ``realloc`` where the return value is assigned to the
   same expression as passed to the first argument.
 
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`cppcoreguidelines-avoid-do-while
+  <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
+
+  Warns when using ``do-while`` loops.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst

+.. title:: clang-tidy - cppcoreguidelines-avoid-do-while
+
+cppcoreguidelines-avoid-do-while
+================================
+
+Warns when using ``do-while`` loops. They are less readable than plain ``while``
+loops, since the termination condition is at the end and the condition is not
+checked prior to the first iteration. This can lead to subtle bugs.
+
+The check implements
+`rule ES.75 of C++ Core Guidelines <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-do>`_.
+
+Examples:
+
+.. code-block:: c++
+
+  int x;
+  do {
+      std::cin >> x;
+      // ...
+  } while (x < 0);
+
+Options
+-------
+
+.. option:: IgnoreMacros
+
+  Ignore the check when analyzing macros. This is useful for safely defining function-like macros:
+
+  .. code-block:: c++
+
+    #define FOO_BAR(x) \
+    do { \
+      foo(x); \
+      bar(x); \
+    } while(0)
+
+  Defaults to `false`.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `clang-analyzer-osx.cocoa.NonNilReturnValue <clang-analyzer/osx.cocoa.NonNilReturnValue.html>`_,
    `clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak <clang-analyzer/osx.cocoa.RunLoopAutoreleaseLeak.html>`_,
    `clang-analyzer-valist.CopyToSelf <clang-analyzer/valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer/valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer/valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency/mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
+   `cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines/avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines/init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines/interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp

+// RUN: %check_clang_tidy -check-suffixes=DEFAULT       %s cppcoreguidelines-avoid-do-while %t
+// RUN: %check_clang_tidy -check-suffixes=IGNORE-MACROS %s cppcoreguidelines-avoid-do-while %t -- -config='{CheckOptions: [{key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true}]}'
+
+#define FOO(x) \
+  do { \
+  } while (x != 0)
+
+#define BAR_0(x) \
+  do { \
+    bar(x); \
+  } while (0)
+
+#define BAR_FALSE(x) \
+  do { \
+    bar(x); \
+  } while (false)
+
+void bar(int);
+int baz();
+
+void foo()
+{
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+    do {
+
+    } while(0);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    do {
+
+    } while(1);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    do {
+
+    } while(-1);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    do {
+
+    } while(false);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    do {
+
+    } while(true);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+    int x1{0};
+    do {
+      x1 = baz();
+    } while (x1 > 0);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    do {
+
+    } while (x1 != 0);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+    constexpr int x2{0};
+    do {
+
+    } while (x2);
+
+    // CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+    constexpr bool x3{false};
+    do {
+
+    } while (x3);
+
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    FOO(x1);
+
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    BAR_0(x1);
+
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+    BAR_FALSE(x1);
+}