hwright added inline comments.
================ Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12 + // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1)) + x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] ---------------- JonasToth wrote: > hwright wrote: > > JonasToth wrote: > > > From this example starting: > > > > > > - The RHS should be a nested expression with function calls, as the RHS > > > is transformed to create the adversary example i mean in the > > > transformation function above. > > > > > > ``` > > > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) > > > - absl::ToDoubleSeconds(d1)); > > > ``` > > > I think you need the proper conversion function, as the result of the > > > expression is `double` and you need a `Duration`, right? > > > > > > But in principle starting from this idea the transformation might break. > > I think there may be some confusion here (and that's entirely my fault. :) ) > > > > We should never get this expression as input to the check, since it doesn't > > compile (as you point out): > > ``` > > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - > > absl::ToDoubleSeconds(d1)); > > ``` > > > > Since `absl::ToDoubleSeconds` requires that its argument is an > > `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - > > absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as > > input. > > > > There may be other expressions which could be input, but in practice they > > don't really happen. I've added a contrived example to the tests, but at > > some point the tests get too complex and confuse the fix matching > > infrastructure. > Your last sentence is the thing ;) Murphies Law will hit this check, too. In > my opinion wrong transformations are very unfortunate and should be avoided > if possible (in this case possible). > You can simply require that the expression of type double does not contain > any duration subtraction calls. > > This is even possible in the matcher-part of the check. I've written a test (which the testing infrastructure fails to handle well, so I haven't included it in the diff), and it produces these results: ``` // // - x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5); + x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5)); // // - x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5)); + x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - absl::Seconds(5)))); ``` Those results are correct. There is a cosmetic issue of round tripping through the `double` conversion in the `absl::Seconds(absl::ToDoubleSeconds(...))` phrase, but untangling that is 1) difficult (because of order of operations issues) and thus; 2) probably the subject of a separate check. This is still such a rare case (as in, we've not encountered it in Google's codebase), that I'm not really concerned. But if it's worth it to explicitly exclude it from the traversal matcher, I can do that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55245/new/ https://reviews.llvm.org/D55245 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
