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:
> 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.
I would agree that they are complicated statements, which is why there are
multi-line comments explaining what is being doing. Moving a two-line compound
statement into a separate function which is only called once seems more
confusing than simplifying. More information can be expressed in a prose
comment than a single concise function name, no?
================
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:
> 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.
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
