[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+      getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
+          "e",
----------------
hwright wrote:
> JonasToth wrote:
> > In Principle the `Node` could have multiple expressions that are a call if there is nesting. 
> > The transformation is correct from what I see right now, but might result in the necessity of multiple passes for the check. (Is the addition version affected from that too?)
> > 
> > Given the recursive nature of the matcher you could construct a nesting with the inner part being a subtraction, too. The generated fixes would conflict and none of them would be applied. At least thats what I would expect right now. Please take a look at this issue.
> There isn't an imminent addition version at this point.
> 
> This matcher isn't recursive: it's just looking at the entire node to see if it is a call to the inverse function.  If an inverse is embedded as part of a deeper expression, it won't see it (e.g., there no `hasDescendant` in this matcher).
Matchers are recursive. There will be a next match of the inner nodes below this node, just by AST traversal.


================
Comment at: clang-tidy/abseil/DurationSubtractionCheck.cpp:38
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID())
+    return;
----------------
`Loc.isInvalid()` too. You can pass in macros from the command line that result in invalid sourcelocations.


================
Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {
----------------
hwright wrote:
> JonasToth wrote:
> > I think having the extraction of the common test-stuff into this header as one commit would be better. Would you prepare such a patch? I can commit for you. It probably makes sense if you ask for commit access (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as you wish.
> I can do this, but it might take a bit to get the commit bit turned on.
Maybe, I (or someone else) have no problem commiting for you.


================
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:
> > 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.


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

https://reviews.llvm.org/D55245