hwright marked 2 inline comments as done. 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: > > > 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. > Can you say what the direct issue is? I would bet its the overlapping? > A note in the documentation would be ok from my side. When the conflicting > transformations are tried to be applied clang-tidy does not crash but print a > nice diagnostic and continue its life? Another example I've verified: ``` - x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1) - 5)); + x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5)); ``` This a nested case, and while `clang-tidy` finds both of them, it only applies the outer most one (presumably the one it finds first in its AST traversal): ``` note: this fix will not be applied because it overlaps with another fix ``` The new code can then be checked again to fix the internal instance. It's not possible to express this case in a test because testing infrastructure uses regular expressions, and the repeated strings in the test expectation cause it to get a bit confused. Given all the of the above, I'm unsure what content would go in the documentation which is specific to this check. 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
