JonasToth added inline comments.
================ 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 = ---------------- hwright wrote: > JonasToth wrote: > > Could you please split this function up into smaller ones. There are three > > or four distinct cases that are easier to comprehend in isolation. > The actual bodies of these if-statements are only one or two separate > statements themselves. Moving those to separate functions seems like it > would just obfuscate things a bit. IMHO they are complicated statements and hide what is being done. Wrapping them in a function with a name that states what is done seems appropriate. ================ 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. +/// ---------------- hwright wrote: > JonasToth wrote: > > nit: 'Time' domain > This doesn't refer to a type, but a library system, so it probably isn't > appropriate to quote it. > > (Just has how one wouldn't quote "frequency" when talking about "the > frequency domain" of a Fourier transform.) ah true, but then time would be written small i guess. ================ 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] ---------------- hwright wrote: > JonasToth wrote: > > 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. > This doesn't actually matter in this case: `x` will be wrapped in a function > call. > > It does matter in the case where we //unwrap// the first argument (below) and > I've already got a test which uses multiplication in this case. I've also > added one for division. Yes, it should not matter if `x` is an expr itself or just a variable. Thats why it should be tested its actually true. 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