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

[Alexander Kornienko]

In http://reviews.llvm.org/D9528#177874, @danielmarjamaki wrote:

> Here are updated results I made today with latest patch:
>
> https://drive.google.com/file/d/0BykPmWrCOxt2S2lkZWpOSEJKUHM/view


This time I found just one issue: in `for (...; X; ...) ...` X doesn't need parentheses. Though it looks better, the check is rather noisy and without a way to fix the issues automatically it will require too much cleanup efforts to be used on any real code base. It's also difficult to point out false positives just by looking at macro definitions.

But if the check provided fix-it hints, it would be easier to test it on real code and it would also be possible to clean up code before starting to use the check.

WDYT?


================
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)
----------------
nit: "Don't"

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

================
Comment at: clang-tidy/misc/MacroParenthesesCheck.cpp:158
@@ +157,3 @@
+
+    // Initialisation.
+    if (Next == tok::l_paren)
----------------
nit: Initialization.

================
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.
> 
> ```
> 
Fair enough. Thanks for trying.

http://reviews.llvm.org/D9528

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