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/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
