[PATCH] clang-tidy: check for repeated side effects in macro

Alexander Kornienko alexfh at google.com
Fri May 8 07:08:24 PDT 2015


Sorry for the delay. I was on vacation and currently digging out stuff out of my inbox. I'll try to review this thoroughly early next week. A few random comments for now.

In http://reviews.llvm.org/D9496#168953, @danielmarjamaki wrote:

> Now it doesn't warn about macros in system headers.


It looks like the latest patch is exactly the same as the first one.


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:41
@@ +40,3 @@
+                                            const MacroArgs *Args) {
+  clang::SourceLocation Loc = Range.getBegin();
+  // Ignore macro argument expansions.
----------------
The `clang::` qualifier is not needed here and below.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:50
@@ +49,3 @@
+  typedef clang::MacroInfo::arg_iterator ArgIter;
+  for (ArgIter AI = MI->arg_begin(), AE = MI->arg_end(); AI != AE; ++AI) {
+    const clang::Token *ResultArgToks =
----------------
Will a range-based for loop work here?

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:54
@@ +53,3 @@
+    int CountInMacro = 0;
+    for (TokIter TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE;
+         ++TI) {
----------------
ditto

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:13
@@ +12,3 @@
+
+void plusplus2() {
+  int j = 0;
----------------
Do you need a separate function for each test?

Also, please leave the whole warning message once and truncate all the other instances to fit into 80 columns.

http://reviews.llvm.org/D9496

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






More information about the cfe-commits mailing list