[clang-tidy] Add readability-avoid-unconditional-preprocessor-if check

[PiotrZSL]

Summary

Check flags always enabled or disabled code blocks in preprocessor '#if'
conditions, such as '#if 0' and '#if 1' etc.

[PiotrZSL]

Micro fixes in documentation

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp

30

Please don't use auto unless type is spelled explicitly in same statement or iterator.

44

Excessive newline.

clang-tools-extra/docs/ReleaseNotes.rst

140

Add etc. at the end?

clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst

8

Ditto.

[PiotrZSL]

Review comments fixes.

[PiotrZSL]

There are 186 findings from this check in llvm repository, no false-positives.

Example:

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:2071:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp:400:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]

[carlosgalvezp]

Looks good, minor comments!

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h

17

Ultra nit: remove extra space

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp

18–32

For completeness, would it make sense to add the same test but with the "is always 'false'" case?

68–70

Add case for if 10 < DDD ?

Also, add test case for comparing 2 macros? If people follow the "no magic numbers" policy they'll likely have defines for both sides of the comparison.

[PiotrZSL]

Removed some whitespace & added tests

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp

68–70

I don't support currently macros, would need to check if they defined unconditionally in same file, and I didn't want to mess with this.

[carlosgalvezp]

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp

41

"static" is an overloaded C++ keyword which can lead to confusion in this context (you don't intend to detect whether a token is the static keyword). Would it be possible to give this a different name? Same for isStaticToken

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp

68–70

Yes I didn't mean "function-like macros", just a plain define like you just added. Great, thanks!

[PiotrZSL]

Change name of isStatic* to isImmutable*

[carlosgalvezp]

LGTM thank you! Really useful check! I'm not very expert in the PP callbacks so I can't give much feedback, feel free to wait for other more expert reviewers if you want.

[carlosgalvezp]

I noticed the pre-merge checks are red, however, would be good to get them fixed.

[PiotrZSL]

@carlosgalvezp Do you want me to shorten documentation ? Or we leave it like it is ?

[PiotrZSL]

Rebase + Shorten documentation

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

139

Please omit Check.

clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst

7

Ditto.

[PiotrZSL]

Doc update

[PiotrZSL]

Fix doc

[carlosgalvezp]

In D145617#4221369, @PiotrZSL wrote:

@carlosgalvezp Do you want me to shorten documentation ? Or we leave it like it is ?

It's fine for me either way!

[carlosgalvezp]

LGTM, thanks!

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h

+//===--- AvoidUnconditionalPreprocessorIfCheck.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_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Check flags always enabled or disabled code blocks in preprocessor `#if`
+/// conditions, such as `#if 0` and `#if 1`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.html
+class AvoidUnconditionalPreprocessorIfCheck : public ClangTidyCheck {
+public:
+  AvoidUnconditionalPreprocessorIfCheck(StringRef Name,
+                                        ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp

+//===--- AvoidUnconditionalPreprocessorIfCheck.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 "AvoidUnconditionalPreprocessorIfCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+namespace {
+struct AvoidUnconditionalPreprocessorIfPPCallbacks : public PPCallbacks {
+
+  explicit AvoidUnconditionalPreprocessorIfPPCallbacks(ClangTidyCheck &Check,
+                                                       Preprocessor &PP)
+      : Check(Check), PP(PP) {}
+
+  void If(SourceLocation Loc, SourceRange ConditionRange,
+          ConditionValueKind ConditionValue) override {
+    if (ConditionValue == CVK_NotEvaluated)
+      return;
+    SourceManager &SM = PP.getSourceManager();
+    if (!isStatic(SM, PP.getLangOpts(), ConditionRange))
+      return;
+
+    if (ConditionValue == CVK_True)
+      Check.diag(Loc, "preprocessor condition is always 'true', consider "
+                      "removing condition but leaving its contents");
+    else
+      Check.diag(Loc, "preprocessor condition is always 'false', consider "
+                      "removing both the condition and its contents");
+  }
+
+  bool isStatic(SourceManager &SM, const LangOptions &LangOpts,
+                SourceRange ConditionRange) {
+    SourceLocation Loc = ConditionRange.getBegin();
+    if (Loc.isMacroID())
+      return false;
+
+    Token Tok;
+    if (Lexer::getRawToken(Loc, Tok, SM, LangOpts, true)) {
+      std::optional<Token> TokOpt = Lexer::findNextToken(Loc, SM, LangOpts);
+      if (!TokOpt || TokOpt->getLocation().isMacroID())
+        return false;
+      Tok = *TokOpt;
+    }
+
+    while (Tok.getLocation() <= ConditionRange.getEnd()) {
+      if (!isStaticToken(Tok))
+        return false;
+
+      std::optional<Token> TokOpt =
+          Lexer::findNextToken(Tok.getLocation(), SM, LangOpts);
+      if (!TokOpt || TokOpt->getLocation().isMacroID())
+        return false;
+      Tok = *TokOpt;
+    }
+
+    return true;
+  }
+
+  bool isStaticToken(const Token &Tok) {
+    switch (Tok.getKind()) {
+    case tok::eod:
+    case tok::eof:
+    case tok::numeric_constant:
+    case tok::char_constant:
+    case tok::wide_char_constant:
+    case tok::utf8_char_constant:
+    case tok::utf16_char_constant:
+    case tok::utf32_char_constant:
+    case tok::string_literal:
+    case tok::wide_string_literal:
+    case tok::comment:
+      return true;
+    case tok::raw_identifier:
+      return (Tok.getRawIdentifier() == "true" ||
+              Tok.getRawIdentifier() == "false");
+    default:
+      return Tok.getKind() >= tok::l_square && Tok.getKind() <= tok::caretcaret;
+    }
+  }
+
+  ClangTidyCheck &Check;
+  Preprocessor &PP;
+};
+
+} // namespace
+
+void AvoidUnconditionalPreprocessorIfCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
+      std::make_unique<AvoidUnconditionalPreprocessorIfPPCallbacks>(*this,
+                                                                    *PP));
+}
+
+} // namespace clang::tidy::readability

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

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
+  AvoidUnconditionalPreprocessorIfCheck.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerContainsCheck.cpp
   ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
   DuplicateIncludeCheck.cpp

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
+#include "AvoidUnconditionalPreprocessorIfCheck.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerContainsCheck.h"
 #include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
 #include "DuplicateIncludeCheck.h"

 namespace clang::tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
+    CheckFactories.registerCheck<AvoidUnconditionalPreprocessorIfCheck>(
+        "readability-avoid-unconditional-preprocessor-if");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ConstReturnTypeCheck>(
         "readability-const-return-type");
     CheckFactories.registerCheck<ContainerContainsCheck>(
         "readability-container-contains");
     CheckFactories.registerCheck<ContainerDataPointerCheck>(
         "readability-container-data-pointer");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved
   the function body.
 
 - New :doc:`llvmlibc-inline-function-decl
   <clang-tidy/checks/llvmlibc/inline-function-decl>` check.
 
   Checks that all implicit and explicit inline functions in header files are
   tagged with the ``LIBC_INLINE`` macro.
 
+- New :doc:`readability-avoid-unconditional-preprocessor-if
+  <clang-tidy/checks/readability/avoid-unconditional-preprocessor-if>` check.
+
+  Check flags always enabled or disabled code blocks in preprocessor ``#if``
+  conditions, such as ``#if 0`` and ``#if 1`` etc.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c

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

.. csv-table::
    `performance-trivially-destructible <performance/trivially-destructible.html>`_, "Yes"
    `performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn.html>`_, "Yes"
    `performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization.html>`_, "Yes"
    `performance-unnecessary-value-param <performance/unnecessary-value-param.html>`_, "Yes"
    `portability-restrict-system-includes <portability/restrict-system-includes.html>`_, "Yes"
    `portability-simd-intrinsics <portability/simd-intrinsics.html>`_,
    `portability-std-allocator-const <portability/std-allocator-const.html>`_,
    `readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls.html>`_, "Yes"
+   `readability-avoid-unconditional-preprocessor-if <readability/avoid-unconditional-preprocessor-if.html>`_,
    `readability-braces-around-statements <readability/braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability/const-return-type.html>`_, "Yes"
    `readability-container-contains <readability/container-contains.html>`_, "Yes"
    `readability-container-data-pointer <readability/container-data-pointer.html>`_, "Yes"
    `readability-container-size-empty <readability/container-size-empty.html>`_, "Yes"
    `readability-convert-member-functions-to-static <readability/convert-member-functions-to-static.html>`_, "Yes"
    `readability-delete-null-pointer <readability/delete-null-pointer.html>`_, "Yes"
    `readability-duplicate-include <readability/duplicate-include.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst

+.. title:: clang-tidy - readability-avoid-unconditional-preprocessor-if
+
+readability-avoid-unconditional-preprocessor-if
+===============================================
+
+Check flags always enabled or disabled code blocks in preprocessor ``#if``
+conditions, such as ``#if 0`` and ``#if 1`` etc.
+
+.. code-block:: c++
+
+    #if 0
+        // some disabled code
+    #endif
+
+    #if 1
+       // some enabled code that can be disabled manually
+    #endif
+
+Preprocessor directives are a powerful feature in C and C++ that allow code to
+be modified before it is compiled. These directives can be used to include or
+exclude code based on conditions, or to define macros that can be used
+throughout the code. However, the misuse of preprocessor directives can create
+several issues that can impact the readability and maintainability of the code.
+
+One issue that can arise from the use of preprocessor directives is dead code.
+Dead code refers to code that is never executed or used during runtime. This can
+occur when code is wrapped in a preprocessor directive that is always disabled,
+such as ``#if 0``. Dead code can make the code harder to read and understand,
+and can create clutter that makes it harder to identify and modify the active
+parts of the code.
+
+Dead code can also create maintenance issues. Code that is never executed or
+used can indicate unfinished or abandoned features, or can create confusion for
+future developers who may try to modify or remove it. This can lead to a
+decrease in the quality and reliability of the codebase, and can make it harder
+to maintain and update over time.
+
+Another issue that can arise from the use of preprocessor directives is the
+creation of additional code branches and paths that need to be tested and
+debugged. Unconditional preprocessor directives that enable code, such as
+``#if 1``, can create additional paths through the code that need to be tested
+and debugged, potentially leading to additional errors or issues.
+
+In the case of unconditional preprocessor directives that enable code, such as
+``#if 1``, there can be a risk of creating always enabled code that may cause
+issues when different parts of the codebase are meant to work together but can
+be enabled separately. This can result in broken functionality if different
+parts of the code are enabled without proper coordination. In such cases, it is
+recommended to use preprocessor macros or defines to control the enabling of the
+code, such as ``#ifdef DEBUGGING_ENABLED``.
+
+Using preprocessor macros or defines can help to prevent issues that may arise
+from the unconditional enabling of code, making it easier to coordinate
+different parts of the codebase and to prevent broken functionality.
+
+By using conditional preprocessor directives with meaningful expressions or
+preprocessor macros instead of unconditional directives that enable code,
+developers can make the codebase more readable, understandable, and maintainable.
+This approach can also help identify unfinished or abandoned features, reduce
+dead code, and minimize the number of additional code branches and paths that
+require testing and debugging. Ultimately, this can lead to a more reliable and
+efficient codebase.
+
+Overall, the use of preprocessor directives should be approached with care to
+ensure that the codebase remains readable, maintainable, and reliable. Avoiding
+the use of unconditional preprocessor directives that enable code and instead
+utilizing conditional preprocessor directives with meaningful expressions or
+preprocessor macros can help to make the codebase easier to read, understand,
+and maintain, ultimately leading to a more reliable and efficient development
+process.
+
+In summary, this check helps detect the use of unconditional preprocessor
+directives that can create technical debt, such as dead code or always enabled
+code. By identifying these issues, this check helps improve the maintainability
+and readability of the codebase.

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp

+// RUN: %check_clang_tidy %s readability-avoid-unconditional-preprocessor-if %t
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]
+#if 0
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
+#if 1
+// some code
+#endif
+
+#if test
+
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
+#if 10>5
+
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]
+#if 10<5
+
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
+#if 10 > 5
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]
+#if 10 < 5
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]
+#if !(10 > \
+        5)
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
+#if !(10 < \
+        5)
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
+#if true
+// some code
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]
+#if false
+// some code
+#endif
+
+#define MACRO
+#ifdef MACRO
+// some code
+#endif
+
+#if !SOMETHING
+#endif
+
+#if !( \
+     defined MACRO)
+// some code
+#endif
+
+
+#if __has_include(<string_view>)
+// some code
+#endif
+
+#if __has_include(<string_view_not_exist>)
+// some code
+#endif
+
+#define DDD 17
+#define EEE 18
+
+#if 10 > DDD
+// some code
+#endif
+
+#if 10 < DDD
+// some code
+#endif
+
+#if EEE > DDD
+// some code
+#endif