JonasToth 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] ---------------- 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? 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
