[PATCH] clang-tidy checker that enforce proper parentheses in macros

[Daniel Marjamäki]

> Did you encounter any cases when the code broke after applying fixes?


No I did not. The projects compiled fine after the 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 can't say exactly right now. It's a little less as far as I know.

> I would also like to see an estimate of the false positive rate (from a random sample of 100 warnings).


I fixed 300 warnings with -fix and saw no compiler warnings.

However I looked at 100 random warnings and saw 5 fp (for type definitions)! I can try to write a heuristic for some of those.


================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:141
@@ +140,3 @@
+
+    // Dont warn after hash/hashhash or before hashhash.
+    if (Prev == tok::hash || Prev == tok::hashhash || Next == tok::hashhash)
----------------
alexfh wrote:
> nit: "Don't"
Fixed

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:149
@@ +148,3 @@
+
+    // String concatenation
+    if (isStringLiteral(Prev) || isStringLiteral(Next))
----------------
alexfh wrote:
> nit: Add a trailing period.
Fixed.

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:158
@@ +157,3 @@
+
+    // Initialisation.
+    if (Next == tok::l_paren)
----------------
alexfh wrote:
> nit: Initialization.
ah.. I missed this.. I'll fix it immediately locally..

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:68
@@ +67,3 @@
+    } else if (Indent <= 0 &&
+               (Tok.is(tok::plus) || Tok.is(tok::minus) || Tok.is(tok::star) ||
+                Tok.is(tok::slash) || Tok.is(tok::percent) ||
----------------
alexfh wrote:
> This looks like a reason to move isOneOf from clang::format::FormatToken (tools/clang/lib/Format/FormatToken.h) to clang::Token. It would greatly simplify this and similar conditions.
Yes... I added a new patch today where this code was rewritten. I did not move isOneOf.. do you still I should do it?

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:77
@@ +76,3 @@
+          !Tok.is(tok::minus) && (TI + 1) != TE &&
+          (TI + 1)->is(tok::numeric_constant) && (TI + 2) == TE) {
+        return;
----------------
danielmarjamaki wrote:
> danielmarjamaki wrote:
> > danielmarjamaki wrote:
> > > alexfh wrote:
> > > > danielmarjamaki wrote:
> > > > > alexfh wrote:
> > > > > > `TI + 2 == TE` implies `TI + 1 != TE`. I'd also put it next to `TI == MI->tokens_begin()`.
> > > > > Yes that is implied. But are you sure it's not UB? I wanted to avoid undefined behaviour when creating an out-of-bounds pointer (calculating TI+2 without checking TI+1). is the buffer always padded with extra elements after TE?
> > > > Why does the buffer need to be padded with something when you compare iterators without dereferencing them? I didn't find any violation of the preconditions for random access iterators (http://en.cppreference.com/w/cpp/concept/RandomAccessIterator).
> > > TI is a pointer.
> > > 
> > > I refer to 6.5.6.8 in the C standard (about pointer addition):
> > > 
> > > If both the pointer operand and the result point to elements of the same array object, or one past the last
> > > element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.
> > > 
> > hmm.. maybe I was confused. I thought it was a pointer addition.. however random access iterators should not overflow neither as far as I know..
> I can't find that TI+2 is UB right now in the specification.
> 
> However for information this sample code:
> 
> 
> ```
> #include <vector>
> 
> int main() {
>   std::vector<int> x;
>   x.push_back(123);
>   std::vector<int>::iterator it = x.begin();
>   if (it+2 == x.end()) {}
>   return 0;
> }
> ```
> 
> When I compile and run it with the STL assertions turned on I get a fault..
> 
> ```
> danielm at debian:~$ g++ -D_GLIBCXX_DEBUG -g bad-stl.cpp -o bad-stl                                                                                                         
> danielm at debian:~$ ./bad-stl                                                                                                                    
> /usr/include/c++/4.7/debug/safe_iterator.h:359:error: attempt to advance a                                                                                                       
>     dereferenceable (start-of-sequence) iterator 2 steps, which falls     
>     outside its valid range.
> 
> ```
> 
This code has been rewritten.

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:86
@@ +85,3 @@
+  if (Err) {
+    SourceLocation Loc = MI->tokens_begin()->getLocation();
+    Check->diag(Loc,
----------------
alexfh wrote:
> nit: No need for a variable here, just use the expression below.
Fixed

http://reviews.llvm.org/D9528

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/