hwright 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 =
----------------
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.
================
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.
+///
----------------
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.)
================
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]
----------------
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.
================
Comment at: test/clang-tidy/abseil-time-subtraction.cpp:78
+ // CHECK-FIXES: return absl::FromUnixSeconds(x) - t;
+}
----------------
JonasToth wrote:
> please add tests for templates and macros.
I've add tests for macros, though I'm not sure what cases you have in mind
regarding templates.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58137/new/
https://reviews.llvm.org/D58137
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
