A few nits, but looks good in general. I would also like to clear a couple of
questions before you submit this.
Did you encounter any cases when the code broke after applying fixes? How many
instances of the warning does the latest version of the check produce on the
same set of projects where you saw 47k warnings initially? I would also like to
see an estimate of the false positive rate (from a random sample of 100
warnings).
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:32
@@ +31,3 @@
+private:
+ /// Replacement list with calculations should be enclosed in parentheses
+ void replacementList(const Token &MacroNameTok, const MacroInfo *MI);
----------------
nit: Missing trailing period.
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:35
@@ +34,3 @@
+
+ /// arguments should be enclosed in parentheses
+ void argument(const Token &MacroNameTok, const MacroInfo *MI);
----------------
nit: Missing trailing period, no capitalization.
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:60
@@ +59,3 @@
+
+/// Is given TokenKind a calculation operator?
+bool isCalcOp(tok::TokenKind K) {
----------------
What is a "calculation operator"? How is it different from other binary
operators in this context?
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:89
@@ +88,3 @@
+ --Count;
+ } else if (Count <= 0 && isCalcOp(Tok.getKind())) {
+ // Heuristic for macros that are clearly not intended to be enclosed in
----------------
I'd make this even stricter: if the replacement list contains unbalanced
parentheses/braces/brackets, then it's clearly not meant to be enclosed in
parentheses.
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.h:25
@@ +24,3 @@
+///
+/// When the replacement list has an expression it is recommended to surround
it
+/// with parentheses. This ensures that the macro result is evaluated
----------------
nit: Missing comma after "expression".
================
Comment at: clang-tidy/misc/MacroParenthesesCheck.h:32
@@ +31,3 @@
+/// properly.
+
+class MacroParenthesesCheck : public ClangTidyCheck {
----------------
nit: Remove the empty line, please.
http://reviews.llvm.org/D9528
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
