https://github.com/LegalizeAdulthood updated https://github.com/llvm/llvm-project/pull/210768
>From 52a51cb86ab611bcd2eaf3e556bdd501de5f5566 Mon Sep 17 00:00:00 2001 From: Richard <[email protected]> Date: Thu, 20 Jan 2022 01:19:19 -0700 Subject: [PATCH] [clang-tidy] Add bugprone-macro-condition check Warns about inconsistent macro usage in preprocessor conditions. #define USE_FOO 0 #ifdef USE_FOO // ...no preprocessor directives testing the value of USE_FOO #endif Here USE_FOO is defined to a value (and furthermore defined to evaluate to false) but the preprocessor condition only checks for the macro being defined. Fixes #27438 --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../bugprone/MacroConditionCheck.cpp | 439 ++++++++++++++++++ .../clang-tidy/bugprone/MacroConditionCheck.h | 30 ++ clang-tools-extra/docs/ReleaseNotes.md | 5 + .../checks/bugprone/macro-condition.md | 65 +++ .../docs/clang-tidy/checks/list.md | 1 + .../Inputs/macro-condition-cross-file.h | 3 + .../bugprone/macro-condition-command-line.cpp | 19 + .../bugprone/macro-condition-cross-file.cpp | 8 + .../checkers/bugprone/macro-condition.cpp | 162 +++++++ 11 files changed, 736 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/macro-condition.md create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/macro-condition-cross-file.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-command-line.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-cross-file.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 3aa39d10ceb5d..e39d90a1177da 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -46,6 +46,7 @@ #include "IntegerDivisionCheck.h" #include "InvalidEnumDefaultInitializationCheck.h" #include "LambdaFunctionNameCheck.h" +#include "MacroConditionCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" #include "MisleadingSetterOfReferenceCheck.h" @@ -203,6 +204,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-invalid-enum-default-initialization"); CheckFactories.registerCheck<LambdaFunctionNameCheck>( "bugprone-lambda-function-name"); + CheckFactories.registerCheck<MacroConditionCheck>( + "bugprone-macro-condition"); CheckFactories.registerCheck<MacroParenthesesCheck>( "bugprone-macro-parentheses"); CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 43e85b1407f21..7c023e7458cfd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -50,6 +50,7 @@ add_clang_library(clangTidyBugproneModule STATIC InfiniteLoopCheck.cpp IntegerDivisionCheck.cpp LambdaFunctionNameCheck.cpp + MacroConditionCheck.cpp MacroParenthesesCheck.cpp MacroRepeatedSideEffectsCheck.cpp MisleadingSetterOfReferenceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.cpp new file mode 100644 index 0000000000000..d52b70cc8ccc4 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.cpp @@ -0,0 +1,439 @@ +//===----------------------------------------------------------------------===// +// +// 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 "MacroConditionCheck.h" +#include "clang/Basic/DiagnosticIDs.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/MacroInfo.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallVector.h" +#include <memory> +#include <optional> +#include <string> +#include <utility> + +namespace clang::tidy::bugprone { + +namespace { +class MacroConditionCallbacks : public PPCallbacks { +public: + MacroConditionCallbacks(MacroConditionCheck *Check, const SourceManager &SM, + Preprocessor &PP) + : Check(Check), SM(SM), PP(PP) {} + + void If(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue) override; + void Ifdef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override; + void Ifndef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override; + void Elif(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue, SourceLocation IfLoc) override; + void Elifdef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override; + void Elifdef(SourceLocation Loc, SourceRange ConditionRange, + SourceLocation IfLoc) override; + void Elifndef(SourceLocation Loc, const Token &MacroNameTok, + const MacroDefinition &MD) override; + void Elifndef(SourceLocation Loc, SourceRange ConditionRange, + SourceLocation IfLoc) override; + void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override; + +private: + enum class ReferenceKind { Definition, Value }; + + struct MacroReference { + std::string Name; + SourceLocation Loc; + ReferenceKind Kind; + }; + + struct MacroUsage { + SourceLocation DefinitionTestLoc; + SourceLocation ValueTestLoc; + bool Diagnosed = false; + }; + + using ConditionReferences = SmallVector<MacroReference, 4>; + + ConditionReferences referencesInCondition(SourceRange ConditionRange) const; + std::optional<std::string> + defaultedMacroInSkippedRange(SourceRange Range) const; + MacroReference referenceFromRange(SourceRange Range, + SourceLocation Loc) const; + bool isIgnoredIdentifier(StringRef Name) const; + void processReferences(const ConditionReferences &References); + void processDefinitionReference(StringRef Name, SourceLocation Loc); + void checkReference(const MacroReference &Reference); + + using FileUsages = llvm::DenseMap<FileID, MacroUsage>; + llvm::DenseMap<const MacroInfo *, FileUsages> MacroUsages; + MacroConditionCheck *Check; + const SourceManager &SM; + Preprocessor &PP; +}; + +} // namespace + +static StringRef getTokenName(const Token &Tok) { + if (Tok.is(tok::raw_identifier)) + return Tok.getRawIdentifier(); + if (const IdentifierInfo *Info = Tok.getIdentifierInfo()) + return Info->getName(); + return {}; +} + +static bool skipFunctionLikeInvocation(ArrayRef<Token> Tokens, size_t &Index) { + if (Index + 1 >= Tokens.size() || Tokens[Index + 1].isNot(tok::l_paren)) + return false; + + unsigned ParenthesisDepth = 0; + do { + ++Index; + if (Tokens[Index].is(tok::l_paren)) + ++ParenthesisDepth; + else if (Tokens[Index].is(tok::r_paren)) + --ParenthesisDepth; + } while (Index + 1 < Tokens.size() && ParenthesisDepth != 0); + return true; +} + +static bool isStandardPredefinedMacro(StringRef Name) { + return Name == "__cplusplus" || Name == "__DATE__" || Name == "__FILE__" || + Name == "__LINE__" || Name == "__TIME__" || + Name.starts_with("__cpp_") || Name.starts_with("__STDC_") || + Name.starts_with("__STDCPP_"); +} + +MacroConditionCallbacks::ConditionReferences +MacroConditionCallbacks::referencesInCondition( + SourceRange ConditionRange) const { + ConditionReferences References; + const SourceLocation BeginLoc = SM.getExpansionLoc(ConditionRange.getBegin()); + if (BeginLoc.isInvalid()) + return References; + + const std::pair<FileID, unsigned> Decomposed = SM.getDecomposedLoc(BeginLoc); + bool Invalid = false; + StringRef Buffer = SM.getBufferData(Decomposed.first, &Invalid); + if (Invalid || Decomposed.second >= Buffer.size()) + return References; + + size_t End = Decomposed.second; + while (End < Buffer.size()) { + if (Buffer[End] != '\r' && Buffer[End] != '\n') { + ++End; + continue; + } + + const size_t Newline = End; + if (Newline > Decomposed.second && Buffer[Newline - 1] == '\\') { + if (Buffer[End] == '\r' && End + 1 < Buffer.size() && + Buffer[End + 1] == '\n') + ++End; + ++End; + continue; + } + break; + } + + std::string Text = Buffer.slice(Decomposed.second, End).str(); + Lexer Lex(BeginLoc, PP.getLangOpts(), Text.data(), Text.data(), + Text.data() + Text.size()); + SmallVector<Token, 16> Tokens; + Token Tok; + bool AtEnd = false; + do { + AtEnd = Lex.LexFromRawLexer(Tok); + if (Tok.isNot(tok::eof)) + Tokens.push_back(Tok); + } while (!AtEnd); + + for (size_t Index = 0; Index < Tokens.size(); ++Index) { + const Token &Current = Tokens[Index]; + if (!Current.is(tok::raw_identifier)) + continue; + + StringRef Name = Current.getRawIdentifier(); + if (Name != "defined") { + if (skipFunctionLikeInvocation(Tokens, Index)) + continue; + if (!isIgnoredIdentifier(Name)) + References.push_back( + {Name.str(), Current.getLocation(), ReferenceKind::Value}); + continue; + } + + const SourceLocation DefinedLoc = Current.getLocation(); + ++Index; + if (Index < Tokens.size() && Tokens[Index].is(tok::l_paren)) + ++Index; + if (Index < Tokens.size() && Tokens[Index].is(tok::raw_identifier)) + References.push_back({Tokens[Index].getRawIdentifier().str(), DefinedLoc, + ReferenceKind::Definition}); + } + return References; +} + +std::optional<std::string> +MacroConditionCallbacks::defaultedMacroInSkippedRange(SourceRange Range) const { + const SourceLocation BeginLoc = SM.getExpansionLoc(Range.getBegin()); + const SourceLocation EndLoc = SM.getExpansionLoc(Range.getEnd()); + if (BeginLoc.isInvalid() || EndLoc.isInvalid()) + return std::nullopt; + + const std::pair<FileID, unsigned> Begin = SM.getDecomposedLoc(BeginLoc); + const std::pair<FileID, unsigned> End = SM.getDecomposedLoc(EndLoc); + if (Begin.first != End.first || Begin.second >= End.second) + return std::nullopt; + + bool Invalid = false; + StringRef Buffer = SM.getBufferData(Begin.first, &Invalid); + if (Invalid || End.second > Buffer.size()) + return std::nullopt; + + std::string Text = Buffer.slice(Begin.second, End.second).str(); + Lexer Lex(BeginLoc, PP.getLangOpts(), Text.data(), Text.data(), + Text.data() + Text.size()); + SmallVector<Token, 32> Tokens; + Token Tok; + bool AtEnd = false; + do { + AtEnd = Lex.LexFromRawLexer(Tok); + if (Tok.isNot(tok::eof)) + Tokens.push_back(Tok); + } while (!AtEnd); + + if (Tokens.size() < 3 || Tokens[0].isNot(tok::hash) || + !Tokens[0].isAtStartOfLine()) + return std::nullopt; + + size_t Index = 2; + std::string Name; + StringRef Directive = getTokenName(Tokens[1]); + if (Directive == "ifndef") { + if (!Tokens[Index].is(tok::raw_identifier)) + return std::nullopt; + Name = Tokens[Index++].getRawIdentifier().str(); + } else if (Directive == "if") { + if (Tokens[Index++].isNot(tok::exclaim) || Index >= Tokens.size() || + getTokenName(Tokens[Index++]) != "defined") + return std::nullopt; + if (Index < Tokens.size() && Tokens[Index].is(tok::l_paren)) + ++Index; + if (Index >= Tokens.size() || Tokens[Index].isNot(tok::raw_identifier)) + return std::nullopt; + Name = Tokens[Index++].getRawIdentifier().str(); + if (Index < Tokens.size() && Tokens[Index].is(tok::r_paren)) + ++Index; + } else { + return std::nullopt; + } + + if (Index < Tokens.size() && !Tokens[Index].isAtStartOfLine()) + return std::nullopt; + + unsigned Depth = 1; + for (; Index + 1 < Tokens.size(); ++Index) { + if (Tokens[Index].isNot(tok::hash) || !Tokens[Index].isAtStartOfLine()) + continue; + + StringRef NestedDirective = getTokenName(Tokens[++Index]); + if (NestedDirective == "if" || NestedDirective == "ifdef" || + NestedDirective == "ifndef") { + ++Depth; + continue; + } + if (NestedDirective == "endif") { + if (--Depth == 0) + break; + continue; + } + if (Depth != 1) + continue; + if (NestedDirective == "else" || NestedDirective.starts_with("elif")) + break; + if (NestedDirective != "define" || Index + 2 >= Tokens.size() || + getTokenName(Tokens[Index + 1]) != Name || + Tokens[Index + 2].isAtStartOfLine()) + continue; + + const Token &MacroName = Tokens[Index + 1]; + const Token &Replacement = Tokens[Index + 2]; + const SourceLocation MacroNameEnd = Lexer::getLocForEndOfToken( + MacroName.getLocation(), 0, SM, PP.getLangOpts()); + if (Replacement.is(tok::l_paren) && + MacroNameEnd == Replacement.getLocation()) + continue; + return Name; + } + return std::nullopt; +} + +MacroConditionCallbacks::MacroReference +MacroConditionCallbacks::referenceFromRange(SourceRange Range, + SourceLocation Loc) const { + ConditionReferences References = referencesInCondition(Range); + if (!References.empty()) { + References.front().Loc = Loc; + References.front().Kind = ReferenceKind::Definition; + return std::move(References.front()); + } + return {{}, Loc, ReferenceKind::Definition}; +} + +bool MacroConditionCallbacks::isIgnoredIdentifier(StringRef Name) const { + const IdentifierInfo *Info = PP.getIdentifierInfo(Name); + return Name == "true" || Name == "false" || + Info->isCPlusPlusOperatorKeyword(); +} + +void MacroConditionCallbacks::processReferences( + const ConditionReferences &References) { + for (const MacroReference &Reference : References) { + bool IsCompoundReference = false; + for (const MacroReference &Other : References) { + if (Reference.Name == Other.Name && Reference.Kind != Other.Kind) { + IsCompoundReference = true; + break; + } + } + if (!IsCompoundReference) + checkReference(Reference); + } +} + +void MacroConditionCallbacks::processDefinitionReference(StringRef Name, + SourceLocation Loc) { + if (!Name.empty()) + checkReference({Name.str(), Loc, ReferenceKind::Definition}); +} + +void MacroConditionCallbacks::checkReference(const MacroReference &Reference) { + if (isStandardPredefinedMacro(Reference.Name)) + return; + + const IdentifierInfo *Info = PP.getIdentifierInfo(Reference.Name); + const MacroInfo *Macro = PP.getMacroDefinition(Info).getMacroInfo(); + if (!Macro || Macro->isBuiltinMacro() || Macro->isFunctionLike() || + Macro->tokens().empty()) + return; + + const SourceLocation SpellingLoc = SM.getSpellingLoc(Reference.Loc); + if (SpellingLoc.isInvalid()) + return; + + MacroUsage &Usage = MacroUsages[Macro][SM.getFileID(SpellingLoc)]; + const bool IsDefinition = Reference.Kind == ReferenceKind::Definition; + SourceLocation &CurrentLoc = + IsDefinition ? Usage.DefinitionTestLoc : Usage.ValueTestLoc; + const SourceLocation OtherLoc = + IsDefinition ? Usage.ValueTestLoc : Usage.DefinitionTestLoc; + if (CurrentLoc.isInvalid()) + CurrentLoc = Reference.Loc; + if (Usage.Diagnosed || OtherLoc.isInvalid()) + return; + + const unsigned Kind = IsDefinition ? 0 : 1; + Check->diag(Reference.Loc, + "Macro '%0' checked here for %select{definition|value}1 after " + "being checked for %select{value|definition}1") + << Reference.Name << Kind; + Check->diag(OtherLoc, + "Macro '%0' first checked here for " + "%select{value|definition}1", + DiagnosticIDs::Note) + << Reference.Name << Kind; + Usage.Diagnosed = true; +} + +void MacroConditionCallbacks::If(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue) { + processReferences(referencesInCondition(ConditionRange)); +} + +void MacroConditionCallbacks::Ifdef(SourceLocation Loc, + const Token &MacroNameTok, + const MacroDefinition &MD) { + processDefinitionReference(getTokenName(MacroNameTok), Loc); +} + +void MacroConditionCallbacks::Ifndef(SourceLocation Loc, + const Token &MacroNameTok, + const MacroDefinition &MD) { + processDefinitionReference(getTokenName(MacroNameTok), Loc); +} + +void MacroConditionCallbacks::Elif(SourceLocation Loc, + SourceRange ConditionRange, + ConditionValueKind ConditionValue, + SourceLocation IfLoc) { + processReferences(referencesInCondition(ConditionRange)); +} + +void MacroConditionCallbacks::Elifdef(SourceLocation Loc, + const Token &MacroNameTok, + const MacroDefinition &MD) { + processDefinitionReference(getTokenName(MacroNameTok), Loc); +} + +void MacroConditionCallbacks::Elifdef(SourceLocation Loc, + SourceRange ConditionRange, + SourceLocation IfLoc) { + MacroReference Reference = referenceFromRange(ConditionRange, Loc); + if (!Reference.Name.empty()) + checkReference(Reference); +} + +void MacroConditionCallbacks::Elifndef(SourceLocation Loc, + const Token &MacroNameTok, + const MacroDefinition &MD) { + processDefinitionReference(getTokenName(MacroNameTok), Loc); +} + +void MacroConditionCallbacks::Elifndef(SourceLocation Loc, + SourceRange ConditionRange, + SourceLocation IfLoc) { + MacroReference Reference = referenceFromRange(ConditionRange, Loc); + if (!Reference.Name.empty()) + checkReference(Reference); +} + +void MacroConditionCallbacks::SourceRangeSkipped(SourceRange Range, + SourceLocation EndifLoc) { + std::optional<std::string> Name = defaultedMacroInSkippedRange(Range); + if (!Name) + return; + + const IdentifierInfo *Info = PP.getIdentifierInfo(*Name); + const MacroInfo *Macro = PP.getMacroDefinition(Info).getMacroInfo(); + auto MacroIt = MacroUsages.find(Macro); + if (MacroIt == MacroUsages.end()) + return; + + const SourceLocation GuardLoc = SM.getSpellingLoc(Range.getBegin()); + const FileID GuardFile = SM.getFileID(GuardLoc); + auto FileIt = MacroIt->second.find(GuardFile); + if (FileIt == MacroIt->second.end()) + return; + + SourceLocation &DefinitionLoc = FileIt->second.DefinitionTestLoc; + if (DefinitionLoc.isValid() && SM.getSpellingLineNumber(DefinitionLoc) == + SM.getSpellingLineNumber(GuardLoc)) + DefinitionLoc = {}; +} + +void MacroConditionCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks(std::make_unique<MacroConditionCallbacks>(this, SM, *PP)); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.h b/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.h new file mode 100644 index 0000000000000..ab4fb90490268 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MacroConditionCheck.h @@ -0,0 +1,30 @@ +//===----------------------------------------------------------------------===// +// +// 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_BUGPRONE_MACROCONDITIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MACROCONDITIONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Warns about inconsistent macro usage in preprocessor conditions. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone-macro-condition.html +class MacroConditionCheck : public ClangTidyCheck { +public: + using ClangTidyCheck::ClangTidyCheck; + + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MACROCONDITIONCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index c2ee1b06c43e0..042986304f503 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -129,6 +129,11 @@ infrastructure are described first, followed by tool-specific sections. #### New checks +- New {doc}`bugprone-macro-condition + <clang-tidy/checks/bugprone/macro-condition>` check. + + Warns about inconsistent macro usage in preprocessor conditions. + - New {doc}`llvm-invalid-regex-pattern <clang-tidy/checks/llvm/invalid-regex-pattern>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/macro-condition.md b/clang-tools-extra/docs/clang-tidy/checks/bugprone/macro-condition.md new file mode 100644 index 0000000000000..8e5006ea20183 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/macro-condition.md @@ -0,0 +1,65 @@ +```{title} clang-tidy - bugprone-macro-condition +``` + +# bugprone-macro-condition + +Warns about inconsistent macro usage in preprocessor conditions. + +Given the following code: + +```c++ +#define USE_FOO 0 +// ... +#if defined(USE_FOO) + // ... +#endif +// ... +#if USE_FOO + // ... +#endif +``` + +`USE_FOO` is checked for definition in one condition and for value in +another. Was the intention to evaluate `USE_FOO` for a `true` expression, +or was the intention to merely check whether the macro was defined? + +The check warns when the same active macro definition is tested both for +definition and for value within the same source file. Conditional uses from +different files are not compared. It does not warn merely because a macro +with a replacement value is tested for definition. Tests of undefined +macros for value are handled by `-Wundef`. + +Compound conditions that test the same macro for both definition and value +are treated as one coherent test and ignored. Standard predefined macros, +including `__cplusplus` and the `__cpp_*`, `__STDC_*`, and `__STDCPP_*` +families, are also ignored. + +A guard that supplies a default value for a macro is not considered a +definition test. For example, this does not qualify for a warning: + +```c++ +#ifndef FEATURE +#define FEATURE 0 +#endif + +#if FEATURE + // ... +#endif +``` + +The same exception applies when the guard is written as +``#if !defined(FEATURE)``. + +No fixes are offered because the intended semantics are ambiguous. + +To resolve a warning, decide which property of the macro is important: + +- If the macro's value is important, keep the value in its definition and + refactor definition tests to test the value, for example with + ``#if USE_FOO`` or an explicit comparison. +- If the macro's presence or absence is important, make it a presence-only + macro and refactor value tests to use ``defined(USE_FOO)`` or + ``!defined(USE_FOO)`` consistently. + +Function-like macros are ignored, including identifiers passed in their +argument lists. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.md b/clang-tools-extra/docs/clang-tidy/checks/list.md index a74a26e691053..8fd3d7408c9f5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.md +++ b/clang-tools-extra/docs/clang-tidy/checks/list.md @@ -118,6 +118,7 @@ readability/* | {doc}`bugprone-integer-division <bugprone/integer-division>` | | | {doc}`bugprone-invalid-enum-default-initialization <bugprone/invalid-enum-default-initialization>` | | | {doc}`bugprone-lambda-function-name <bugprone/lambda-function-name>` | | +| {doc}`bugprone-macro-condition <bugprone/macro-condition>` | | | {doc}`bugprone-macro-parentheses <bugprone/macro-parentheses>` | Yes | | {doc}`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>` | | | {doc}`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>` | | diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/macro-condition-cross-file.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/macro-condition-cross-file.h new file mode 100644 index 0000000000000..13418c41a6e8f --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/macro-condition-cross-file.h @@ -0,0 +1,3 @@ +#ifndef CROSS_FILE_MACRO +#define CROSS_FILE_MACRO 1 +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-command-line.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-command-line.cpp new file mode 100644 index 0000000000000..6ceb899592473 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-command-line.cpp @@ -0,0 +1,19 @@ +// RUN: %check_clang_tidy -check-suffix=DEFINED %s \ +// RUN: bugprone-macro-condition %t -- -- -DCOMMAND_LINE_MACRO=0 +// RUN: clang-tidy %s -checks=-*,bugprone-macro-condition -- \ +// RUN: -UCOMMAND_LINE_MACRO | count 0 + +// With -UCOMMAND_LINE_MACRO, these conditions are equivalent to: +// +// #undef COMMAND_LINE_MACRO + +// With -DCOMMAND_LINE_MACRO=0, they are equivalent to: +// +// #define COMMAND_LINE_MACRO 0 +#ifdef COMMAND_LINE_MACRO +#endif + +#if COMMAND_LINE_MACRO +// CHECK-MESSAGES-DEFINED: :[[@LINE-1]]:5: warning: Macro 'COMMAND_LINE_MACRO' checked here for value after being checked for definition +// CHECK-MESSAGES-DEFINED: :[[@LINE-5]]:2: note: Macro 'COMMAND_LINE_MACRO' first checked here for definition +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-cross-file.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-cross-file.cpp new file mode 100644 index 0000000000000..4fbb2e8fc29b3 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition-cross-file.cpp @@ -0,0 +1,8 @@ +// RUN: clang-tidy %s -checks=-*,bugprone-macro-condition -- -I %S | count 0 + +#define CROSS_FILE_MACRO 1 +#include "Inputs/macro-condition-cross-file.h" + +#if CROSS_FILE_MACRO +#endif + diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition.cpp new file mode 100644 index 0000000000000..3e0eab3576876 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-condition.cpp @@ -0,0 +1,162 @@ +// RUN: %check_clang_tidy %s bugprone-macro-condition %t + +#define USE_FOO 0 + +#if defined(USE_FOO) +void f() +{ + extern void foo(); + foo(); +} +#endif + +#define VALUE_DEFINED 42 +#ifndef VALUE_DEFINED +#endif +// CHECK-MESSAGES-NOT: warning: Macro 'VALUE_DEFINED' + +#if 0 +#elif OTHER_MACRO +#elifdef OTHER_MACRO2 +#else +#endif + +#if !defined(USE_FOO) +void f2() +{ + extern void notFoo(); + notFoo(); +} +#endif + +#ifdef USE_FOO +void f3() +{ + extern void foo(); + foo(); +} +#endif + +#ifndef USE_FOO +void f4() +{ + extern void notFoo(); + notFoo(); +} +#endif + +#if 0 +#elif defined(USE_FOO) +void f5() +{ + extern void foo(); + foo(); +} +#endif + +// CHECK-MESSAGES-NOT: warning: Macro 'USE_FOO' +// CHECK-MESSAGES-NOT: warning: Undefined macro 'OTHER_MACRO' + +#define USE_GRONK 0 +#ifdef USE_GRONK +#if USE_GRONK +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Macro 'USE_GRONK' checked here for value after being checked for definition +// CHECK-MESSAGES: :[[@LINE-3]]:2: note: Macro 'USE_GRONK' first checked here for definition +void f6() +{ + extern void foo(); + foo(); +} +#endif +#endif + +#define VALUE_FIRST 0 +#if VALUE_FIRST +#endif +#ifndef VALUE_FIRST +// CHECK-MESSAGES: :[[@LINE-1]]:2: warning: Macro 'VALUE_FIRST' checked here for definition after being checked for value +// CHECK-MESSAGES: :[[@LINE-4]]:5: note: Macro 'VALUE_FIRST' first checked here for value +void f7() +{ + extern void foo(); + foo(); +} +#endif + +#define SAME_CONDITION 0 +#if defined(SAME_CONDITION) && SAME_CONDITION +void f8() +{ + extern void foo(); + foo(); +} +#endif + +#if __has_include(<sys/file.h>) +#include <sys/file.h> +#endif +// CHECK-MESSAGES-NOT: warning: Undefined macro 'sys' checked here for value +// CHECK-MESSAGES-NOT: warning: Undefined macro 'file' checked here for value +// CHECK-MESSAGES-NOT: warning: Undefined macro 'h' checked here for value + +#if __has_builtin(__builtin_trap) +#endif +// CHECK-MESSAGES-NOT: warning: Undefined macro '__builtin_trap' checked here for value + +#if __has_cpp_attribute(gnu::always_inline) +#endif +// CHECK-MESSAGES-NOT: warning: Undefined macro 'gnu' checked here for value +// CHECK-MESSAGES-NOT: warning: Undefined macro 'always_inline' checked here for value + +#define ALWAYS_TRUE(x) 1 +#if ALWAYS_TRUE(not_a_macro) +#endif +// CHECK-MESSAGES-NOT: warning: Undefined macro 'not_a_macro' checked here for value + +#ifdef ALWAYS_TRUE +#endif +// CHECK-MESSAGES-NOT: warning: Macro 'ALWAYS_TRUE' defined here with a value and checked for definition + +#define GUARDED 1 +#if defined(GUARDED) +#undef GUARDED +#if GUARDED +#endif +#endif +// CHECK-MESSAGES-NOT: warning: {{.*}}'GUARDED' + +#define GUARDED_CONJUNCTION 1 +#ifdef GUARDED_CONJUNCTION +#endif +#if defined(GUARDED_CONJUNCTION) && GUARDED_CONJUNCTION +#endif + +#define GUARDED_DISJUNCTION 1 +#ifdef GUARDED_DISJUNCTION +#endif +#if !defined(GUARDED_DISJUNCTION) || GUARDED_DISJUNCTION +#endif + +#ifdef __cplusplus +#endif +#if __cplusplus >= 201103L +#endif + +#ifdef __STDC_HOSTED__ +#endif +#if __STDC_HOSTED__ +#endif + +#define DEFAULT_IFNDEF 1 +#ifndef DEFAULT_IFNDEF +#define DEFAULT_IFNDEF 0 +#endif +#if DEFAULT_IFNDEF +#endif + +#define DEFAULT_NOT_DEFINED 1 +#if !defined(DEFAULT_NOT_DEFINED) +#define DEFAULT_NOT_DEFINED 0 +#endif +#if DEFAULT_NOT_DEFINED +#endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
