This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add the abseil-duration-addition check
ClosedPublic

Authored by hwright on Jan 24 2019, 1:32 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hwright created this revision.Jan 24 2019, 1:32 PM
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
76 ↗(On Diff #183387)

Please use alphabetical order for new checks.

JonasToth added inline comments.Jan 25 2019, 2:33 AM
clang-tidy/abseil/DurationAdditionCheck.cpp
22 ↗(On Diff #183387)

This whole file is structurally similar to the DurationSubtractionCheck.cpp equivalent (which is expected :)), maybe some parts could be refactored out?
Will there be a check for duration scaling or the like?

50 ↗(On Diff #183387)

The only difference between the two diag seems to be the FixItHint. I would rather refactor the diag out of the if.

60 ↗(On Diff #183387)

What happens with paren-casts, are they ignored as well? (See one comment in the test-cases)

test/clang-tidy/abseil-duration-addition.cpp
28 ↗(On Diff #183387)

Call is on the right side, what happens for 3 + (abs::ToUnixHours(t));

48 ↗(On Diff #183387)

Is something like this

i += absl::ToInt64MicroSeconds(absl::Seconds(1));

possible (syntactically and semantically)?
The compound operators are currently not covered by this (and the subtraction-check), but in this case it looks like a possible use-case.

hwright updated this revision to Diff 183531.Jan 25 2019, 6:23 AM

Sort release notes

hwright updated this revision to Diff 183539.Jan 25 2019, 7:19 AM
hwright marked 11 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/DurationAdditionCheck.cpp
22 ↗(On Diff #183387)

I think that most of it already has been factored out (e.g., rewriteExprFromNumberToDuration and getScaleForTimeInverse, etc) . The actual meat here is pretty light: the matcher, and then the diagnostic generation.

A scaling change may also follow. Our experience has been that scaling isn't quite straight forward, as the scaling factor may have semantic meaning which changes the result, but which isn't captured by the type system. Probably not a design discussion to have here, but suffice it to say that I don't know if this is yet settled.

50 ↗(On Diff #183387)

This doesn't really de-dupe very much, since the bulk of the work here is creating the FixItHint. Done.

60 ↗(On Diff #183387)

Now they are.

test/clang-tidy/abseil-duration-addition.cpp
28 ↗(On Diff #183387)

Added this test, and updated to ignore paren casts.

48 ↗(On Diff #183387)

Semantically, this is possible, but wouldn't fall under this transform, since it would require changing the type of i.

JonasToth added inline comments.Jan 25 2019, 7:28 AM
clang-tidy/abseil/DurationAdditionCheck.cpp
22 ↗(On Diff #183387)

Your right, there is not too much to gain. Only if there are more checks coming with the same structure it makes sense to think again.

test/clang-tidy/abseil-duration-addition.cpp
84 ↗(On Diff #183539)

a view template test-cases would be good to have.

hwright marked 2 inline comments as done.Jan 25 2019, 7:45 AM
hwright added inline comments.
test/clang-tidy/abseil-duration-addition.cpp
84 ↗(On Diff #183539)

I'm not sure I know that terminology; do you have an example?

JonasToth added inline comments.Jan 25 2019, 8:09 AM
test/clang-tidy/abseil-duration-addition.cpp
84 ↗(On Diff #183539)
template<typename T>
void foo(absl::Time t) {
  int i = absl::ToUnixNanos(t) + T{};
}
foo<int>(t);
foo<double>(t);
hwright updated this revision to Diff 183583.Jan 25 2019, 11:17 AM
hwright marked an inline comment as done.

Add another test

hwright marked 2 inline comments as done.Jan 25 2019, 11:17 AM
This revision is now accepted and ready to land.Jan 28 2019, 2:55 AM
This revision was automatically updated to reflect the committed changes.