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

[Daniel Marjamäki]

In http://reviews.llvm.org/D9528#176259, @alexfh wrote:

> A few random issues I noticed on the first couple of pages of results you posted on a different thread:
>
>   ../src/transmit.c:37:25: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses]
>   #define MKQUERY_ADDB(b) *rqp++= (b)
>                           ^
>   
>
> It looks like the check is complaining about  the `rgp` variable, which is not a parameter of the macro.
>
>   ../src/addrfam.c:538:3: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses]
>     const char *p= dgram + rps->labstart[labnum]; \
>     ^
>   
>
> Does it complain about `const`? Then it looks like the same issue as the first one.
>
>   adnstest.c:82:3: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses]
>     case e: fprintf(stderr,"adns failure: %s: errno=" #e "\n",what); break
>     ^
>   adnstest.c:82:8: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
>     case e: fprintf(stderr,"adns failure: %s: errno=" #e "\n",what); break
>          ^
>
>
> Same.
>
> It looks like a frequent case, so fixing this would help fixing lots of false positives.
>
>   ../src/addrfam.c:47:48: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
>   #define SIN(cnst, sa) ((void)(sa)->sa_family, (cnst struct sockaddr_in *)(sa))
>                                                  ^
>   
>
> `cnst` is a parameter of the macro, but parens are not needed (if acceptable) in this context. The rule only makes sense for the macro arguments that can be expressions, so you should try to restrict the check to the cases where macro argument resembles an expression or when the context where the parameter is expanded looks like an expression. I'm not sure how exactly this can be done, and whether it's possible to do this precisely enough, but some cases can definitely be improved.


Hmm.. I think that the results are a bit confusing. When it says "macro replacement list should be enclosed in parentheses" I want that the whole expression is surrounded with parentheses.

> It looks like the check is complaining about  the `rgp` variable, which is not a parameter of the macro.


Yes it is confusing.. maybe it shouldn't point at the rgp.

> Does it complain about `const`? Then it looks like the same issue as the first one.


no.. but I do think it's a confusing message.

> `cnst` is a parameter of the macro, but parens are not needed (if acceptable) in this context. The rule only makes sense for the macro arguments that can be expressions, so you should try to restrict the check to the cases where macro argument resembles an expression or when the context where the parameter is expanded looks like an expression. I'm not sure how exactly this can be done, and whether it's possible to do this precisely enough, but some cases can definitely be improved.


I agree .. maybe it's acceptable to hide the warning when the next token is a name.


================
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 MacroDirective *MD);
----------------
alexfh wrote:
> Here and below. Please use C++ style comments (`//` or `///` for Doxygen comments). The comments should be English prose with proper capitalization and punctuation.
ok will try to fix in next patch

http://reviews.llvm.org/D9528

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