This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add the abseil-duration-factory-float check
ClosedPublic

Authored by hwright on Oct 16 2018, 1:52 PM.

Details

Summary

This check finds cases where calls to an absl::Duration factory could use the more efficient integer overload.

For example:
// Original - Providing a floating-point literal.
absl::Duration d = absl::Seconds(10.0);

// Suggested - Use an integer instead.
absl::Duration d = absl::Seconds(10);

Diff Detail

Repository
rL LLVM

Event Timeline

hwright created this revision.Oct 16 2018, 1:52 PM

LG In general, but see a few comments inline.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
25 ↗(On Diff #169887)
27 ↗(On Diff #169887)

Probably doesn't matter much, but would std::modf be more appropriate in this context?

65–66 ↗(On Diff #169887)

Lexer::makeFileCharRange may be a better (and less strict) way to test whether we can safely replace a range.

87 ↗(On Diff #169887)

const auto * will be as readable and less verbose.

docs/ReleaseNotes.rst
73 ↗(On Diff #169887)

Please remove the FIXME

alexfh edited reviewers, added: hokein, aaron.ballman; removed: alexfh_.Oct 16 2018, 4:53 PM
Eugene.Zelenko added inline comments.
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
24 ↗(On Diff #169887)

Please use static keyword instead of anonymous namespace.

70 ↗(On Diff #169887)

May be Macros are ignored will sound better?

docs/ReleaseNotes.rst
70 ↗(On Diff #169887)

Please sort new check lists alphabetically.

docs/clang-tidy/checks/abseil-duration-factory-float.rst
8 ↗(On Diff #169887)

Please fix double space.

Eugene.Zelenko retitled this revision from Add the abseil-duration-factory-float clang-tidy check to [clang-tidy] Add the abseil-duration-factory-float check.Oct 16 2018, 5:06 PM
Eugene.Zelenko added a reviewer: JonasToth.
Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.
Eugene.Zelenko added a project: Restricted Project.

By the word, why this check could not be generalized to any function/method which have floating-point and integer variants?

hwright updated this revision to Diff 169946.Oct 16 2018, 8:41 PM
hwright marked 8 inline comments as done.

Addressed review comments

hwright added inline comments.Oct 16 2018, 8:43 PM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
27 ↗(On Diff #169887)

I'm not actually sure. Since we're checking the remainder against 0, we don't need to also separately get the integral part, since if the conditional passes, we know the original value is the integral part. It would seem that std::modf just adds more complexity.

JonasToth added inline comments.Oct 17 2018, 5:44 AM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
34 ↗(On Diff #169946)

Wouldn't it make more sense to use std::uint64_t instead to correspond to the line above? And where does the signedness difference come from? (/*isUnsigned=*/false)

61 ↗(On Diff #169946)

That is worth a helper function taking a SourceRange as argument.

69 ↗(On Diff #169946)

maybe assert instead, as your comment above suggests that macros are already filtered out?

72 ↗(On Diff #169946)

missing full stop.

84 ↗(On Diff #169946)

missing full stop

docs/clang-tidy/checks/list.rst
12 ↗(On Diff #169946)

spurious change

test/clang-tidy/abseil-duration-factory-float.cpp
32 ↗(On Diff #169946)

Could you also provide test cases with hexadecimal floating literals, which are C++17? The thousand-separators could be checked as well (dunno the correct term right now, but the 1'000'000 feature).
Please add test cases for negative literals, too.

zturner added inline comments.
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
24 ↗(On Diff #169946)

All variables (local, global, function parameter) use exactly same naming convention CamelCase.

hwright updated this revision to Diff 170083.Oct 18 2018, 7:15 AM
hwright marked 6 inline comments as done.

Addressed reviewer comments.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
34 ↗(On Diff #169946)

I don't remember where the signedness difference came from, so I've made this std::int64_t.

69 ↗(On Diff #169946)

This is a different check than above.

In the first case, we want to be sure we aren't replacing cases inside of a macro, such as:

#define SECONDS(x) absl::Seconds(x)
SECONDS(1.0)

In this one, we want to avoid changing the argument if it is itself a macro:

#define VAL 1.0
absl::Seconds(VAL);

So it is a separate check, not just a re-assertion of the first one.

test/clang-tidy/abseil-duration-factory-float.cpp
32 ↗(On Diff #169946)

Added the hexadecimal floating literal tests.

The testing infrastructure wouldn't build the test source with 3'000 as a literal argument. (There's also an argument that by the time we get to the AST, that distinction is gone anyway and this test isn't the place to check comprehensive literal parsing.)

I've also added a negative literal test, to illustrate that we don't yet handle that case (which is in practice pretty rare). I'd rather add it in a separate change.

JonasToth added inline comments.Oct 19 2018, 5:07 AM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
69 ↗(On Diff #169946)

Ok, I misunderstood the code then :)

test/clang-tidy/abseil-duration-factory-float.cpp
32 ↗(On Diff #169946)

I am happy with that. I agree that parsing should deal with it fine and your code seems to do it fine as well. My experience is, that sometimes there are still surprises :)

Ping.

What are the next steps here?

Ping.

What are the next steps here?

Please mark all comments you consider resolved as 'Done', i think alex already kinda accepted it, but given there were more comments he should take another look.

hwright marked 9 inline comments as done.Oct 22 2018, 4:25 AM
JonasToth added inline comments.Oct 22 2018, 1:44 PM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
51 ↗(On Diff #170083)

What about c-style casts?

67 ↗(On Diff #170083)

Please clarfiy this comment a bit more, like Marcros as arguments are ignored. or the like.

86 ↗(On Diff #170083)

please highlight the code construct ('Duration' in this case) here and in other comments to clarify its about the class in user-code.

clang-tidy/abseil/DurationFactoryFloatCheck.h
19 ↗(On Diff #170083)

Please add more to the doc here, like This check finds ... and transforms these calls into ... or similar.

docs/clang-tidy/checks/abseil-duration-factory-float.rst
6 ↗(On Diff #170083)

Please synchronize the first paragraph with the release notes (I would prefer the release notes version)

hwright updated this revision to Diff 170659.Oct 23 2018, 8:39 AM
hwright marked 5 inline comments as done.

Address reviewer comments

clang-tidy/abseil/DurationFactoryFloatCheck.h
19 ↗(On Diff #170083)

I've added some more text.

I'd prefer not to make this duplicate much of what is already in the user-facing documentation, since that should be more complete and canonical (and is already included by reference).

hwright marked an inline comment as done.Oct 23 2018, 8:40 AM
JonasToth accepted this revision.EditedOct 23 2018, 9:30 AM

@alexfh you did comment before, do you want to add more? I have no issues left.

Please give alex the opportunity to react, but if he doesn't (he has a lot to do) you can commit in 3 days or so. Do you have commit access?

EDIT: did you run this check over abseil code? Did you notice anything unregular?

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
51 ↗(On Diff #170659)

Nit: the duplication in the cast matcher can be removed with a variable auto CastToFloat = hasDestinationType(realFloatingPointType()), hasSourceExpression(expr().bind("cast_arg")); and then used in the matcher.

99 ↗(On Diff #170659)

is it logically valid to fall reach the end of the method?
If not please add an llvm_unreachable()

This revision is now accepted and ready to land.Oct 23 2018, 9:30 AM
hwright marked 2 inline comments as done.Oct 23 2018, 11:26 AM

I do not have commit privileges, so somebody else would need to submit it.

We've had a version of this check running over our internal codebase for several months with no unexpected problems.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
51 ↗(On Diff #170659)

This turns out to be harder than it looks: auto can't be used here because it would be ambiguous, and the actual type is part of the internal namespace so I'm hesitant to use it here.

I've left the duplication as-is; if there's a better way to de-dupe, I'm happy to try it.

99 ↗(On Diff #170659)

It is. Any case we don't catch would fall through to the end of this function.

hwright marked 4 inline comments as done.Oct 23 2018, 11:26 AM
aaron.ballman added inline comments.Oct 23 2018, 11:39 AM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
29–31 ↗(On Diff #170659)

I believe you can do return APSInt::get(static_cast<int64_t>(Value)); instead -- it should do the same thing.

36 ↗(On Diff #170659)

This function name doesn't seem to relate to the behavior of the function? Rather than try to pin it down like that, perhaps rename to "CanRangeBeSafelyReplaced()` and add a comment as to why this is the correct approach for answering the question.

54 ↗(On Diff #170659)

What about function-style casts? e.g. float(1)

74 ↗(On Diff #170659)

Comment is now out of date, this checks for a few kinds of casts.

hwright marked 4 inline comments as done.Oct 23 2018, 6:46 PM
hwright added inline comments.
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
36 ↗(On Diff #170659)

I've added a comment describing what the function does, but leaving the name the same.

Since this isn't the only reason why we reject a given range, and I think keeping those decisions co-located is probably best, I'd like to leave that logic in the check method below, rather than moving part of it up here.

hwright updated this revision to Diff 170803.Oct 23 2018, 6:47 PM
hwright marked an inline comment as done.

Address reviewer comments.

hwright marked an inline comment as done.Oct 23 2018, 6:47 PM

looks good, just a few nits.

clang-tidy/abseil/AbseilTidyModule.cpp
32 ↗(On Diff #170803)

Maybe drop the factory? we already have a duration-related check abseil-duration-division, for consistency.

clang-tidy/rename_check.py may help you.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
94 ↗(On Diff #170803)

nit: clang-tidy message is not a complete sentence, use lower letter use.

hwright updated this revision to Diff 170847.Oct 24 2018, 4:18 AM
hwright marked 2 inline comments as done.

Update diagnostic text

hwright added inline comments.Oct 24 2018, 5:50 AM
clang-tidy/abseil/AbseilTidyModule.cpp
32 ↗(On Diff #170803)

The expectation is that there may be other checks relating to calls to duration factories, beyond just floating point literal and cast simplification.

Though I'm happy to be convinced otherwise.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
94 ↗(On Diff #170803)

This is a complete sentence, in the imperative style of other clang-tidy checks. I've added a full-stop at the end.

hokein accepted this revision.Oct 24 2018, 6:04 AM

LGTM.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
94 ↗(On Diff #170803)

Ah, sorry for not clarifying it clearly. In clang-tidy, we don't use complete sentence for warnings, so just remove the "." at the end.

alexfh added inline comments.Oct 24 2018, 8:42 AM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
84 ↗(On Diff #169946)

Clang-tidy (and clang) diagnostics don't end with a period. This one doesn't need to be special in this regard. Same below.

JonasToth added inline comments.Oct 24 2018, 8:44 AM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
84 ↗(On Diff #169946)

I believe that was related to a comment to make it a full sentence with grammar and so on, AFAIK these are supposed to be correct.
The diagnostic message itself not, I did not intend to include it.

hwright updated this revision to Diff 170912.Oct 24 2018, 9:16 AM
hwright marked 5 inline comments as done.

Remove full-stop

I think accepted now? :)
If you want I can commit for you and monitor the buildbot, if there are bigger problems I would come back to you.

I think accepted now? :)
If you want I can commit for you and monitor the buildbot, if there are bigger problems I would come back to you.

@JonasToth I don't actually have commit privileges, so somebody else will have to commit for me. :)

This revision was automatically updated to reflect the committed changes.

Thanks for the patch!

@JonasToth I don't actually have commit privileges, so somebody else will have to commit for me. :)

You should definitely ask commit access.