hwright added inline comments.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+ using argument_type = DurationScale;
+ unsigned operator()(DurationScale Scale) const {
----------------
JonasToth wrote:
> Are you using `argument_type`? Browser searching did only show one result.
This is required by `IndexedMap`, if I understand correctly.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+ unsigned operator()(DurationScale Scale) const {
+ return static_cast<unsigned>(Scale);
+ }
----------------
JonasToth wrote:
> Why not `std::uint8_t` as its the underlying type for the `enum`?
This is required by `IndexedMap`, if I understand correctly.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+ getInverseForScale(Scale);
+ if (const auto *MaybeCallArg = selectFirst<const Expr>(
+ "e",
----------------
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).
================
Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {
----------------
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.
================
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:
> 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.
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
