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

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 27 04:38:12 PST 2019


JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

I am happy now :)
Thank you for the patch, LGTM



================
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:
> > 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.
> Added, though this seems more a test of the matcher infrastructure than the tool itself.
True, but there are always some inconsistencies, so better to test what we want :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58137/new/

https://reviews.llvm.org/D58137





More information about the cfe-commits mailing list