Page MenuHomePhabricator

[clang-tidy] Expand cases covered by the abseil-duration-unnecessary-conversion check
ClosedPublic

Authored by hwright on Mar 9 2019, 6:17 PM.

Details

Summary

This adds coverage for expressions of these forms

absl::Duration d2, d1;
d2 = absl::Seconds(d1 / absl::Seconds(1));
d2 = absl::Seconds(absl::FDivDuration(d1, absl::Seconds(1)));

Diff Detail

Event Timeline

hwright created this revision.Mar 9 2019, 6:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2019, 6:17 PM
hwright added a project: Restricted Project.Mar 9 2019, 6:18 PM
hokein added inline comments.Mar 12 2019, 2:16 AM
clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
31

could you add a few comment briefly describing these matchers?

test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
86

An off-topic comment: do we have this code pattern in the codebase? From my understanding, the usage like this is really rare.

88

could you check the full statement d2 = d1? the same to other places.

hwright updated this revision to Diff 190456.Mar 13 2019, 11:09 AM
hwright marked 4 inline comments as done.

Addressed reviewer comments

test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
86

This mostly shows up in migrations. When an existing API takes an integer, and caller already has a Duration, they sometimes use dur / absl::Seconds(1) to get back in integer at the callsite. After a migration, we end up with absl::Seconds(dur /absl::Seconds(1)), which should really just be dur.

hokein accepted this revision.Mar 14 2019, 1:45 AM
This revision is now accepted and ready to land.Mar 14 2019, 1:45 AM
This revision was automatically updated to reflect the committed changes.