Page MenuHomePhabricator

[clang-tidy] Add the abseil-duration-conversion-cast check
ClosedPublic

Authored by hwright on Jan 9 2019, 7:13 PM.

Details

Summary

This suggests simplifying expressions which are casting conversion functions, such as static_cast<int>(absl::ToDoubleSeconds(...))

Diff Detail

Repository
rL LLVM

Event Timeline

hwright created this revision.Jan 9 2019, 7:13 PM

nit's come from running running validate_check.py on the rst file from D55523: [clang-tidy] Linting .rst documentation

$ ../clang-tidy/validate_check.py --rst abseil-duration-conversion-cast.rst
Checking abseil-duration-conversion-cast.rst...
warning: line 6 maximize 80 characters by joining:'[Checks for casts of `absl::Duration` conversion functions, and recommends]' and '[the...]

warning: line 20 multiple blank lines
warning: line 28 multiple blank lines

docs/clang-tidy/checks/abseil-duration-conversion-cast.rst
20 ↗(On Diff #180988)

nit: double blank line

28 ↗(On Diff #180988)

nit: double blank line

31 ↗(On Diff #180988)

nit: I believe that 'instead.' can come onto this line and still be <= 80 characters (which might actually be a bug in validate_check.py that it doesn't detect it)

hwright updated this revision to Diff 181125.Jan 10 2019, 12:08 PM
hwright marked 5 inline comments as done.

Update documentation line wrapping.

docs/clang-tidy/checks/abseil-duration-conversion-cast.rst
20 ↗(On Diff #180988)

This is intentional (and consistent with other examples in other tools).

28 ↗(On Diff #180988)

This is intentional (and consistent with other documentation).

Eugene.Zelenko added inline comments.
clang-tidy/abseil/DurationConversionCastCheck.cpp
41 ↗(On Diff #181125)

Please run Clang-format.

Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.Jan 10 2019, 5:36 PM
hwright updated this revision to Diff 181204.Jan 10 2019, 7:00 PM
hwright marked an inline comment as done.

Run clang-format

aaron.ballman added inline comments.Jan 15 2019, 12:23 PM
clang-tidy/abseil/DurationComparisonCheck.cpp
47–48 ↗(On Diff #181204)

I think this change (and the related removal) should be landed separately as a NFC commit once this one lands, as it's logically separate from the new check.

clang-tidy/abseil/DurationConversionCastCheck.cpp
46 ↗(On Diff #181204)

You can drop the llvm:: qualifiers here and elsewhere.

52 ↗(On Diff #181204)

Add a full stop to the end of the comment (same below).

57 ↗(On Diff #181204)

This diagnostic message does not tell the user what they did wrong with their code. How about duration should be converted directly to integer|floating-point rather than through a type cast or something along those lines?

test/clang-tidy/abseil-duration-conversion-cast.cpp
10 ↗(On Diff #181204)

Can you add an example like:

typedef int FancyInt;

FancyInt j = static_cast<FancyInt>(absl::ToDoubleHours(d1));

to make sure the cast is looking through types as expected? Please add a similar test for floating-point casts.

hwright updated this revision to Diff 182043.Jan 16 2019, 7:27 AM
hwright marked 6 inline comments as done.

Address reviewer comments

clang-tidy/abseil/DurationComparisonCheck.cpp
47–48 ↗(On Diff #181204)

I've gone ahead and done the change separately, which should clean this up.

This revision is now accepted and ready to land.Jan 17 2019, 10:55 AM
This revision was automatically updated to reflect the committed changes.