JonasToth added inline comments.
================ Comment at: clang-tidy/abseil/DurationRewriter.cpp:92 + DurationScale Scale, const Expr &Node) { + const llvm::StringRef &InverseFunction = getTimeInverseForScale(Scale); + if (const auto *MaybeCallArg = selectFirst<const Expr>( ---------------- nit: stringref should be used as value all the time (as its a pointer itself). ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:65 + llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(TimeInverse); + if (!Scale) + continue; ---------------- wouldn't that be rather a bug condition? I think `assert` is better in that case ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:72 + + // Match the cases where we know that the result is a Duration and the first + // argument is a Time. Just knowing the type of the first operand is not ---------------- please highlight the types with '' ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:97 +void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) { + const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop"); + std::string inverse_name = ---------------- Could you please split this function up into smaller ones. There are three or four distinct cases that are easier to comprehend in isolation. ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:98 + const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop"); + std::string inverse_name = + Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString(); ---------------- nit: `InverseName` ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:108 + // We're working with the first case of matcher, and need to replace the + // entire Duration factory call. (Which also means being careful about + // our order-of-operations and optionally putting in some parenthesis. ---------------- please elimate the multiple blanks ================ Comment at: clang-tidy/abseil/TimeSubtractionCheck.h:19 +/// Finds and fixes `absl::Time` subtraction expressions to do subtraction +/// in the Time domain instead of the numeric domain. +/// ---------------- nit: 'Time' domain ================ Comment at: test/clang-tidy/abseil-time-subtraction.cpp:12 + + d = absl::Hours(absl::ToUnixHours(t) - x); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction] ---------------- please add tests where `x` itself is a calculation with different precedence of its operators (multiplication, addition) to ensure these cases are transformed properly as well. ================ Comment at: test/clang-tidy/abseil-time-subtraction.cpp:78 + // CHECK-FIXES: return absl::FromUnixSeconds(x) - t; +} ---------------- please add tests for templates and macros. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58137/new/ https://reviews.llvm.org/D58137 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits