[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

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