I have copied the isOneOf from FormatToken. I did not want to move TokenType
etc and therefore the FormatToken still has isOneOf also. As far as I see all
comments in the code has been taken care of.
I've rerun original and latest patch with my script and compared warnings. This
time duplicated warnings was filtered out.
Original patch:
projects: 217
warnings: 3758
Latest patch:
projects: 100
warnings: 2529
The "193 projects" I initially talked about should be a subset of the projects
I checked today.. unless some project has been removed from the debian archive
I'm checking last month.
Latest patch gives more warnings than the original patch. The original checker
stopped the checking of a macro when a problem was found. Now all violations in
the macro are reported (and fixed).
Also when comparing warnings from the original patch and latest patch there are
some warnings that are not reported with latest patch. I don't see false
negatives, only fixed false positives.
http://reviews.llvm.org/D9528
Files:
include/clang/Lex/Token.h
tools/extra/clang-tidy/misc/CMakeLists.txt
tools/extra/clang-tidy/misc/MacroParenthesesCheck.cpp
tools/extra/clang-tidy/misc/MacroParenthesesCheck.h
tools/extra/clang-tidy/misc/MiscTidyModule.cpp
tools/extra/test/clang-tidy/misc-macro-parentheses.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: include/clang/Lex/Token.h
===================================================================
--- include/clang/Lex/Token.h
+++ include/clang/Lex/Token.h
@@ -94,6 +94,13 @@
/// "if (Tok.is(tok::l_brace)) {...}".
bool is(tok::TokenKind K) const { return Kind == K; }
bool isNot(tok::TokenKind K) const { return Kind != K; }
+ bool isOneOf(tok::TokenKind K1, tok::TokenKind K2) const {
+ return is(K1) || is(K2);
+ }
+ template <typename... Ts>
+ bool isOneOf(tok::TokenKind K1, tok::TokenKind K2, Ts... Ks) const {
+ return is(K1) || isOneOf(K2, Ks...);
+ }
/// \brief Return true if this is a raw identifier (when lexing
/// in raw mode) or a non-keyword identifier (when lexing in non-raw mode).
Index: tools/extra/test/clang-tidy/misc-macro-parentheses.cpp
===================================================================
--- tools/extra/test/clang-tidy/misc-macro-parentheses.cpp
+++ 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
Index: tools/extra/clang-tidy/misc/CMakeLists.txt
===================================================================
--- tools/extra/clang-tidy/misc/CMakeLists.txt
+++ tools/extra/clang-tidy/misc/CMakeLists.txt
@@ -7,6 +7,7 @@
BoolPointerImplicitConversionCheck.cpp
InaccurateEraseCheck.cpp
InefficientAlgorithmCheck.cpp
+ MacroParenthesesCheck.cpp
MiscTidyModule.cpp
NoexceptMoveConstructorCheck.cpp
StaticAssertCheck.cpp
Index: tools/extra/clang-tidy/misc/MacroParenthesesCheck.h
===================================================================
--- tools/extra/clang-tidy/misc/MacroParenthesesCheck.h
+++ tools/extra/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: tools/extra/clang-tidy/misc/MacroParenthesesCheck.cpp
===================================================================
--- tools/extra/clang-tidy/misc/MacroParenthesesCheck.cpp
+++ tools/extra/clang-tidy/misc/MacroParenthesesCheck.cpp
@@ -0,0 +1,189 @@
+//===--- 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.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;
+ } 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.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) {
+ 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<MacroParenthesesPPCallbacks>(
+ &Compiler.getPreprocessor(), this));
+}
+
+} // namespace tidy
+} // namespace clang
Index: tools/extra/clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- tools/extra/clang-tidy/misc/MiscTidyModule.cpp
+++ 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 @@
"misc-inaccurate-erase");
CheckFactories.registerCheck<InefficientAlgorithmCheck>(
"misc-inefficient-algorithm");
+ CheckFactories.registerCheck<MacroParenthesesCheck>(
+ "misc-macro-parentheses");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
CheckFactories.registerCheck<StaticAssertCheck>(
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
