hokein added inline comments.
================ Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:24 + + const auto IsDuration = + expr(hasType(cxxRecordDecl(hasName("::absl::Duration")))); ---------------- maybe call it `DurationExpr` since you have declared the variable as `expr(...)`. ================ Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:30 + hasOverloadedOperatorName("/"), + hasArgument(0, expr(IsDuration)), + hasArgument(1, expr(IsDuration))).bind("OpCall"))), ---------------- `expr(IsDuration)` seems have a duplicated `expr`, isn't `hasArgument(0, IsDuration())` enough? ================ Comment at: test/clang-tidy/abseil-duration-division.cpp:18 + +absl::Duration d; + ---------------- could you make it to a local variable? It willmake the test code easier to read, otherwise readers have to scroll up the file to see what is variable `d`. ================ Comment at: test/clang-tidy/abseil-duration-division.cpp:58 + // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration objects + // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return + // absl::FDivDuration(t1, t2);} ---------------- I think we should ignore templates. The template function could be used by other types, fixing the template is not correct. https://reviews.llvm.org/D50389 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits