From 302275a938141ab1af7b7d73007ae2ad3bc802bc Mon Sep 17 00:00:00 2001 From: Daniel Marjamaki Date: Tue, 16 Jun 2015 14:27:31 +0000 Subject: [PATCH] clang-tidy: Add checker that warns about missing parentheses in macros * calculations in the replacement list should be inside parentheses * macro arguments should be inside parentheses llvm-svn: 239820 --- clang-tools-extra/clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MacroParenthesesCheck.cpp | 192 +++++++++++++++++++++ .../clang-tidy/misc/MacroParenthesesCheck.h | 42 +++++ .../clang-tidy/misc/MiscTidyModule.cpp | 3 + .../test/clang-tidy/misc-macro-parentheses.cpp | 36 ++++ 5 files changed, 274 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/misc-macro-parentheses.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 961ffde..d4057bf 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_library(clangTidyMiscModule BoolPointerImplicitConversionCheck.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp + MacroParenthesesCheck.cpp MiscTidyModule.cpp NoexceptMoveConstructorCheck.cpp StaticAssertCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.cpp b/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.cpp new file mode 100644 index 0000000..1649d3f --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.cpp @@ -0,0 +1,192 @@ +//===--- MacroParenthesesCheck.cpp - clang-tidy----------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MacroParenthesesCheck.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" + +namespace clang { +namespace tidy { + +namespace { +class MacroParenthesesPPCallbacks : public PPCallbacks { +public: + explicit MacroParenthesesPPCallbacks(Preprocessor *PP, + MacroParenthesesCheck *Check) + : PP(PP), Check(Check) {} + + void MacroDefined(const Token &MacroNameTok, + const MacroDirective *MD) override { + replacementList(MacroNameTok, MD->getMacroInfo()); + argument(MacroNameTok, MD->getMacroInfo()); + } + +private: + /// Replacement list with calculations should be enclosed in parentheses. + void replacementList(const Token &MacroNameTok, const MacroInfo *MI); + + /// Arguments should be enclosed in parentheses. + void argument(const Token &MacroNameTok, const MacroInfo *MI); + + Preprocessor *PP; + MacroParenthesesCheck *Check; +}; + +/// Is argument surrounded properly with parentheses/braces/squares/commas? +bool isSurroundedLeft(const Token &T) { + return T.isOneOf(tok::l_paren, tok::l_brace, tok::l_square, tok::comma, + tok::semi); +} + +/// Is argument surrounded properly with parentheses/braces/squares/commas? +bool isSurroundedRight(const Token &T) { + return T.isOneOf(tok::r_paren, tok::r_brace, tok::r_square, tok::comma, + tok::semi); +} + +/// Is given TokenKind a keyword? +bool isKeyword(const Token &T) { + /// \TODO better matching of keywords to avoid false positives + return T.isOneOf(tok::kw_case, tok::kw_const, tok::kw_struct); +} + +/// Warning is written when one of these operators are not within parentheses. +bool isWarnOp(const Token &T) { + /// \TODO This is an initial list of operators. It can be tweaked later to + /// get more positives or perhaps avoid some false positive. + return T.isOneOf(tok::plus, tok::minus, tok::star, tok::slash, tok::percent, + tok::amp, tok::pipe, tok::caret); +} +} + +void MacroParenthesesPPCallbacks::replacementList(const Token &MacroNameTok, + const MacroInfo *MI) { + // Count how deep we are in parentheses/braces/squares. + int Count = 0; + + // SourceLocation for error + SourceLocation Loc; + + for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) { + const Token &Tok = *TI; + // Replacement list contains keywords, don't warn about it. + if (isKeyword(Tok)) + return; + // When replacement list contains comma/semi don't warn about it. + if (Count == 0 && Tok.isOneOf(tok::comma, tok::semi)) + return; + if (Tok.isOneOf(tok::l_paren, tok::l_brace, tok::l_square)) { + ++Count; + } else if (Tok.isOneOf(tok::r_paren, tok::r_brace, tok::r_square)) { + --Count; + // If there are unbalanced parentheses don't write any warning + if (Count < 0) + return; + } else if (Count == 0 && isWarnOp(Tok)) { + // Heuristic for macros that are clearly not intended to be enclosed in + // parentheses, macro starts with operator. For example: + // #define X *10 + if (TI == MI->tokens_begin() && (TI + 1) != TE && + !Tok.isOneOf(tok::plus, tok::minus)) + return; + // Don't warn about this macro if the last token is a star. For example: + // #define X void * + if ((TE - 1)->is(tok::star)) + return; + + Loc = Tok.getLocation(); + } + } + if (Loc.isValid()) { + const Token &Last = *(MI->tokens_end() - 1); + Check->diag(Loc, "macro replacement list should be enclosed in parentheses") + << FixItHint::CreateInsertion(MI->tokens_begin()->getLocation(), "(") + << FixItHint::CreateInsertion(Last.getLocation().getLocWithOffset( + PP->getSpelling(Last).length()), + ")"); + } +} + +void MacroParenthesesPPCallbacks::argument(const Token &MacroNameTok, + const MacroInfo *MI) { + + for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) { + // First token. + if (TI == MI->tokens_begin()) + continue; + + // Last token. + if ((TI + 1) == MI->tokens_end()) + continue; + + const Token &Prev = *(TI - 1); + const Token &Next = *(TI + 1); + + const Token &Tok = *TI; + + // Only interested in identifiers. + if (!Tok.isOneOf(tok::identifier, tok::raw_identifier)) + continue; + + // Only interested in macro arguments. + if (MI->getArgumentNum(Tok.getIdentifierInfo()) < 0) + continue; + + // Argument is surrounded with parentheses/squares/braces/commas. + if (isSurroundedLeft(Prev) && isSurroundedRight(Next)) + continue; + + // Don't warn after hash/hashhash or before hashhash. + if (Prev.isOneOf(tok::hash, tok::hashhash) || Next.is(tok::hashhash)) + continue; + + // Argument is a struct member. + if (Prev.isOneOf(tok::period, tok::arrow)) + continue; + + // String concatenation. + if (isStringLiteral(Prev.getKind()) || isStringLiteral(Next.getKind())) + continue; + + // Type/Var. + if (isAnyIdentifier(Prev.getKind()) || isKeyword(Prev) || + isAnyIdentifier(Next.getKind()) || isKeyword(Next)) + continue; + + // Initialization. + if (Next.is(tok::l_paren)) + continue; + + // Cast. + if (Prev.is(tok::l_paren) && Next.is(tok::star) && + TI + 2 != MI->tokens_end() && (TI + 2)->is(tok::r_paren)) + continue; + + // Assignment. + if (Prev.is(tok::equal) && Next.is(tok::semi)) + continue; + + Check->diag(Tok.getLocation(), "macro argument should be enclosed in " + "parentheses") + << FixItHint::CreateInsertion(Tok.getLocation(), "(") + << FixItHint::CreateInsertion(Tok.getLocation().getLocWithOffset( + PP->getSpelling(Tok).length()), + ")"); + } +} + +void MacroParenthesesCheck::registerPPCallbacks(CompilerInstance &Compiler) { + Compiler.getPreprocessor().addPPCallbacks( + llvm::make_unique( + &Compiler.getPreprocessor(), this)); +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.h b/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.h new file mode 100644 index 0000000..36b5789 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MacroParenthesesCheck.h @@ -0,0 +1,42 @@ +//===--- MacroParenthesesCheck.h - clang-tidy--------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_PARENTHESES_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_PARENTHESES_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +/// \brief Finds macros that can have unexpected behaviour due to missing +/// parentheses. +/// +/// Macros are expanded by the preprocessor as-is. As a result, there can be +/// unexpected behaviour; operators may be evaluated in unexpected order and +/// unary operators may become binary operators, etc. +/// +/// When the replacement list has an expression, it is recommended to surround +/// it with parentheses. This ensures that the macro result is evaluated +/// completely before it is used. +/// +/// It is also recommended to surround macro arguments in the replacement list +/// with parentheses. This ensures that the argument value is calculated +/// properly. +class MacroParenthesesCheck : public ClangTidyCheck { +public: + MacroParenthesesCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(CompilerInstance &Compiler) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_PARENTHESES_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 21248d5..91f59f7 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -16,6 +16,7 @@ #include "BoolPointerImplicitConversionCheck.h" #include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" +#include "MacroParenthesesCheck.h" #include "NoexceptMoveConstructorCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" @@ -42,6 +43,8 @@ public: "misc-inaccurate-erase"); CheckFactories.registerCheck( "misc-inefficient-algorithm"); + CheckFactories.registerCheck( + "misc-macro-parentheses"); CheckFactories.registerCheck( "misc-noexcept-move-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/test/clang-tidy/misc-macro-parentheses.cpp b/clang-tools-extra/test/clang-tidy/misc-macro-parentheses.cpp new file mode 100644 index 0000000..9e5a2fe --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-macro-parentheses.cpp @@ -0,0 +1,36 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-macro-parentheses %t +// REQUIRES: shell + +#define BAD1 -1 +// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses] +#define BAD2 1+2 +// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses] +#define BAD3(A) (A+1) +// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses] +#define BAD4(x) ((unsigned char)(x & 0xff)) +// CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses] + +#define GOOD1 1 +#define GOOD2 (1+2) +#define GOOD3(A) #A +#define GOOD4(A,B) A ## B +#define GOOD5(T) ((T*)0) +#define GOOD6(B) "A" B "C" +#define GOOD7(b) A b +#define GOOD8(a) a B +#define GOOD9(type) (type(123)) +#define GOOD10(car, ...) car +#define GOOD11 a[b+c] +#define GOOD12(x) a[x] +#define GOOD13(x) a.x +#define GOOD14(x) a->x +#define GOOD15(x) ({ int a = x; a+4; }) +#define GOOD16(x) a_ ## x, b_ ## x = c_ ## x - 1, +#define GOOD17 case 123: x=4+5; break; +#define GOOD18(x) ;x; +#define GOOD19 ;-2; +#define GOOD20 void* + +// These are allowed for now.. +#define MAYBE1 *12.34 +#define MAYBE2 <<3 -- 2.7.4