Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -7,6 +7,7 @@ BoolPointerImplicitConversionCheck.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp + MacroParenthesesCheck.cpp MiscTidyModule.cpp NoexceptMoveConstructorCheck.cpp StaticAssertCheck.cpp Index: clang-tidy/misc/MacroParenthesesCheck.h =================================================================== --- clang-tidy/misc/MacroParenthesesCheck.h +++ clang-tidy/misc/MacroParenthesesCheck.h @@ -0,0 +1,43 @@ +//===--- 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 Index: clang-tidy/misc/MacroParenthesesCheck.cpp =================================================================== --- clang-tidy/misc/MacroParenthesesCheck.cpp +++ clang-tidy/misc/MacroParenthesesCheck.cpp @@ -0,0 +1,185 @@ +//===--- 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(tok::TokenKind K) { + return K == tok::l_paren || K == tok::l_brace || K == tok::l_square || + K == tok::comma || K == tok::semi; +} + +/// Is argument surrounded properly with parentheses/braces/squares/commas? +bool isSurroundedRight(tok::TokenKind K) { + return K == tok::r_paren || K == tok::r_brace || K == tok::r_square || + K == tok::comma || K == tok::semi; +} + +/// Is given TokenKind a keyword? +bool isKeyword(tok::TokenKind K) { + /// \TODO better matching of keywords + return K == tok::kw_case || K == tok::kw_const || K == tok::kw_struct; +} + +/// Is given TokenKind a calculation operator? +bool isCalcOp(tok::TokenKind K) { + return K == tok::plus || K == tok::minus || K == tok::star || + K == tok::slash || K == tok::percent || K == tok::amp || + K == tok::pipe || K == 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.getKind())) + return; + // When replacement list contains comma/semi don't warn about it. + if (Count <= 0 && (Tok.is(tok::comma) || Tok.is(tok::semi))) + return; + if (Tok.is(tok::l_paren) || Tok.is(tok::l_brace) || Tok.is(tok::l_square)) { + ++Count; + } else if (Tok.is(tok::r_paren) || Tok.is(tok::r_brace) || + Tok.is(tok::r_square)) { + --Count; + } else if (Count <= 0 && isCalcOp(Tok.getKind())) { + // 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.is(tok::plus) && + !Tok.is(tok::minus)) { + 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) { + const Token &Tok = *TI; + + // Only interested in identifiers. + if (!Tok.is(tok::identifier) && !Tok.is(tok::raw_identifier)) + continue; + + // Only interested in macro arguments. + if (MI->getArgumentNum(Tok.getIdentifierInfo()) < 0) + continue; + + // First token. + if (TI == MI->tokens_begin()) + continue; + + // Last token. + if ((TI + 1) == MI->tokens_end()) + continue; + + const tok::TokenKind Prev = (TI - 1)->getKind(); + const tok::TokenKind Next = (TI + 1)->getKind(); + + // 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 == tok::hash || Prev == tok::hashhash || Next == tok::hashhash) + continue; + + // Argument is a struct member. + if (Prev == tok::period || Prev == tok::arrow) + continue; + + // String concatenation. + if (isStringLiteral(Prev) || isStringLiteral(Next)) + continue; + + // Type/Var. + if (isAnyIdentifier(Prev) || isKeyword(Prev) || isAnyIdentifier(Next) || + isKeyword(Next)) + continue; + + // Initialization. + if (Next == tok::l_paren) + continue; + + // Cast. + if (Prev == tok::l_paren && Next == tok::star && + TI + 2 != MI->tokens_end() && (TI + 2)->is(tok::r_paren)) + continue; + + // Assignment. + if (Prev == tok::equal && Next == 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 Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ 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 @@ "misc-inaccurate-erase"); CheckFactories.registerCheck( "misc-inefficient-algorithm"); + CheckFactories.registerCheck( + "misc-macro-parentheses"); CheckFactories.registerCheck( "misc-noexcept-move-constructor"); CheckFactories.registerCheck( Index: test/clang-tidy/misc-macro-parentheses.cpp =================================================================== --- test/clang-tidy/misc-macro-parentheses.cpp +++ test/clang-tidy/misc-macro-parentheses.cpp @@ -0,0 +1,35 @@ +// 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; + +// These are allowed for now.. +#define MAYBE1 *12.34 +#define MAYBE2 <<3