Page MenuHomePhabricator

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

Authored by hwright on Jan 28 2019, 12:51 PM.

Diff Detail

Event Timeline

hwright created this revision.Jan 28 2019, 12:51 PM
Eugene.Zelenko added inline comments.
clang-tidy/abseil/DurationDoubleConversionCheck.cpp
48 ↗(On Diff #183945)

Please run Clang-format.

docs/ReleaseNotes.rst
85

Finds and fixes. Please use `` for language constructs. Same in documentation.

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

Unnecessary empty line.

27 ↗(On Diff #183945)

Unnecessary empty line.

hwright updated this revision to Diff 184143.Jan 29 2019, 11:37 AM
hwright marked 5 inline comments as done.

Address reviewer comments.

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

This is consistent with other documentation in this directory, such as abseil-faster-strsplit-delimiter.rst.

docs/clang-tidy/checks/abseil-duration-double-conversion.rst
28 ↗(On Diff #184143)

Please use `` for language constructs. Same below.

hwright updated this revision to Diff 184146.Jan 29 2019, 11:48 AM
hwright marked 2 inline comments as done.
hwright added inline comments.
docs/clang-tidy/checks/abseil-duration-double-conversion.rst
28 ↗(On Diff #184143)

Thanks; I thought I'd caught them all.

MyDeveloperDay added inline comments.
docs/clang-tidy/checks/abseil-duration-double-conversion.rst
20 ↗(On Diff #183945)

In your example abseil-faster-strsplit-delimiter.rst , The double blank line in the html doesn't give much delineation between the before and after code and the next example.

There probably isn't a convention per say (which is a shame), across the docs we do a mixture of different styles

https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html
https://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html
https://clang.llvm.org/extra/clang-tidy/checks/google-objc-function-naming.html

But there is a desire by some of the regular clang-tidy reviewers to make the documentation consistent

It may not be ideal but the "Before/After" style, that is used in modernize-use-emplace, modernize-use-using,readability-braces-around-statements,readability-identifier-naming and readability-redundant-function-ptr-dereference does help a little.

I'm not saying looks better, but I've added a couple of examples of formatting the strsplit example for comparison, feel free to ignore.

hwright marked 2 inline comments as done.Jan 30 2019, 12:13 PM
hwright added inline comments.
docs/clang-tidy/checks/abseil-duration-double-conversion.rst
20 ↗(On Diff #183945)

I like those examples!

Would it be reasonable to update all of the abseil-duration-* documentation in a separate pass, after this change is submitted?

MyDeveloperDay added inline comments.Jan 31 2019, 2:38 AM
docs/clang-tidy/checks/abseil-duration-double-conversion.rst
20 ↗(On Diff #183945)

If it means the documentation gets better in the long run, I'm fine with that, its mainly a Nit on my part.

hwright added a subscriber: ioeric.
hwright removed a subscriber: ioeric.
hokein added a comment.Feb 4 2019, 3:08 AM

The code looks good, but I have a concern about the check name -- double seems a confusing word, see my comment.

clang-tidy/abseil/DurationDoubleConversionCheck.cpp
27 ↗(On Diff #184146)

nit: s => S; might be inline the Scales?

51 ↗(On Diff #184146)

The double word may cause confusion easily -- at the first glance, I thought that it means convert the absl::Duration to the double type, but it turned out not.. How about using abseil-duration-necessary-conversion?

hwright updated this revision to Diff 185057.Feb 4 2019, 8:02 AM
hwright marked 4 inline comments as done.
hwright retitled this revision from [clang-tidy] Add the abseil-duration-double-conversion check to [clang-tidy] Add the abseil-duration-unnecessary-conversion check.

Renamed to abseil-duration-unnecessary-conversion

@hokein Thanks for the suggestion on the name, I was looking for something a little less confusing.

hokein accepted this revision.Feb 4 2019, 10:39 AM

LGTM

clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
51

also avoid double word in the message.

docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
18

and here.

26

and here.

This revision is now accepted and ready to land.Feb 4 2019, 10:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 11:28 AM