[clang-tidy] Abseil: integral division of Duration check
ClosedPublic

Authored by deannagarcia on Aug 7 2018, 8:34 AM.

Details

Summary

This check is an abseil specific test that tests to ensure users utilize abseil specific floating point division when trying to divide with abseil duration types.

Diff Detail

deannagarcia created this revision.Aug 7 2018, 8:34 AM
deannagarcia created this object with visibility "All Users".
deannagarcia created this object with edit policy "Administrators".

Hi deannagarcia and thank you for your first contribution :)

It is uncommon, that only Administrators are allowed to modify a revision. Usually we directly adjust the Revision to add missing organizational information (like reviewers, ...).

  • Please add this check to the Release Notes.
  • Please add aaron.ballman and hokein as reviewers
  • Please change the title to something like '[clang-tidy] new check for Abs ...', so automatic subscription and stuff works (which is usually based on the [clang-tidy] tag)
  • Please ensure that the patch is attached to the ClangToolsExtra repository
  • Please add cfe-commits to the subscribers.

(You can remove the modification restriction, too)

clang-tidy/abseil/DurationDivisionCheck.cpp
22

Please wrap return onto the next line. clang-format should do that automatically.

24

Please follow the LLVM naming convention (IsDuration in this case, maybe DurationExpr would be better too).
https://llvm.org/docs/CodingStandards.html#the-low-level-issues

33

What about different kinds of casts, like C-Style casts?
Doesn't the hasImplicitDestinationType already remove the possibility for an cast as destination?

38

Please follow the naming convention here as well -> Op or better OpCall or similar to have more telling names.

40

maybe you could add the full name for Duration? Not sure if it will be more telling. Especially if the diagnostic is mixed with non-absl-checks it might not be very clear what Duration is meant?

clang-tidy/abseil/DurationDivisionCheck.h
20

I think potential instead of possible would sound better.

docs/clang-tidy/checks/abseil-duration-division.rst
7

Please you 2 '``' signs for code-constructs in the .rst documentation.

9

Please add one more sentence, why this is something you don't want, so it gets clear that floating point contextes are the interesting here.

21

Maybe its better to use 3.0 in the assert to signify its an floating point value.

test/clang-tidy/abseil-duration-division.cpp
16

Please add examples with template-functions. What is the suggestion there?

template <class T>
void DoGenericStuff(T t);

DoGenericStuff(Duration(10) / Duration(1));
22

What happens on this snippet:

const auto SomeVal = 1.0 + d / d; // auto deduces to a double?! so it should be considered as relevant
const auto AnotherVal = 1 + d / d; // Here everything will be an integer, so interesting as well
24

what about float? I guess it shuold behave identically?

41

What about other integer types like long. What about short, char?

44

Please add c-style casts as well. Even though they are not supposed to be used.

deannagarcia retitled this revision from Abseil Duration Division clang-tidy check to [clang-tidy] new check for Abseil.Aug 7 2018, 10:07 AM
deannagarcia added a reviewer: aaron.ballman.
deannagarcia set the repository for this revision to rCTE Clang Tools Extra.
deannagarcia changed the edit policy from "Administrators" to "All Users".
deannagarcia added a subscriber: cfe-commits.
Eugene.Zelenko edited reviewers, added: alexfh, ilya-biryukov; removed: Restricted Project.Aug 7 2018, 10:22 AM
Eugene.Zelenko added a project: Restricted Project.
deannagarcia marked 7 inline comments as done.Aug 7 2018, 10:48 AM
JonasToth added inline comments.Aug 7 2018, 2:44 PM
clang-tidy/abseil/DurationDivisionCheck.cpp
31

s/is_duration/IsDuration/ twice

38

op still is lowercase.

docs/ReleaseNotes.rst
59

Please add one empty line above and please remove the The improvements are... line.

This might clash with other checks that are in review right now, but yours might be the first one that lands.

hokein added a comment.Aug 8 2018, 5:30 AM

Thanks for contributing to clang-tidy!

docs/clang-tidy/checks/abseil-duration-division.rst
4

This is a nice document. Does abseil have similar thing in its official guideline/practice? If yes, we can add the link in the document as well.

test/clang-tidy/abseil-duration-division.cpp
22

+1, we need to add more test cases.

deannagarcia changed the visibility from "All Users" to "Public (No Login Required)".Aug 9 2018, 8:00 AM
deannagarcia marked 9 inline comments as done.Aug 9 2018, 2:45 PM
deannagarcia added inline comments.
clang-tidy/abseil/DurationDivisionCheck.cpp
33

I know the test fails without this line and flags the casts, so I'm pretty sure it's necessary but I'm not exactly sure why hasImplicitDestinationType doesn't catch it.

docs/clang-tidy/checks/abseil-duration-division.rst
4

I linked our general documentation on absl::Duration, specifically the stuff on duration arithmetic. Is that what you wanted?

9

Does this link work or do you still want more?

alexfh added inline comments.Aug 9 2018, 3:25 PM
docs/clang-tidy/checks/abseil-duration-division.rst
9

See ... for more information on how `absl::Duration` arithmetic functions.

The sentence is incomplete.

JonasToth added inline comments.Aug 10 2018, 2:21 AM
clang-tidy/abseil/DurationDivisionCheck.cpp
33

Ok. I can not help there. Just leave as is :)

docs/clang-tidy/checks/abseil-duration-division.rst
9

Link is enough!

deannagarcia marked 7 inline comments as done.
JonasToth added inline comments.Aug 10 2018, 7:16 AM
clang-tidy/abseil/DurationDivisionCheck.cpp
30

The argumentCountIs should be redundant, not? Operator/ has always two operands and must be overloaded as an binary operator.

32

I dont understand the expr().bind, you can directly bind to the cxxOperatorCallExpr. That should be equivalent.

clang-tidy/abseil/DurationDivisionCheck.h
20

The common For the user-facing documentation see: <LinkToDoc> is missing here.
All clang-tidy checks do have this (as it is autogenerated by the add-check tool) so i think it would be better to stay consistent with it.

deannagarcia marked 3 inline comments as done.

LGTM with only the formatting question.

But don't commit before any of the reviewers accepts (@alexfh @aaron.ballman usually have the last word)

clang-tidy/abseil/DurationDivisionCheck.cpp
51

This line looks odd, does it come from clang-format?

deannagarcia added inline comments.Aug 10 2018, 8:39 AM
clang-tidy/abseil/DurationDivisionCheck.cpp
51

I have run clang-format on this file and it's still formatted like this.

JonasToth added inline comments.Aug 10 2018, 9:06 AM
clang-tidy/abseil/DurationDivisionCheck.cpp
51

Leave as is :)

lebedev.ri retitled this revision from [clang-tidy] new check for Abseil to [clang-tidy] Abseil: integral division of Duration check.Aug 10 2018, 9:08 AM
hokein added inline comments.Aug 13 2018, 1:02 AM
clang-tidy/abseil/DurationDivisionCheck.cpp
25

maybe call it DurationExpr since you have declared the variable as expr(...).

31

expr(IsDuration) seems have a duplicated expr, isn't hasArgument(0, IsDuration()) enough?

test/clang-tidy/abseil-duration-division.cpp
19

could you make it to a local variable? It willmake the test code easier to read, otherwise readers have to scroll up the file to see what is variable d.

59

I think we should ignore templates. The template function could be used by other types, fixing the template is not correct.

deannagarcia marked 6 inline comments as done.
deannagarcia added inline comments.
test/clang-tidy/abseil-duration-division.cpp
59

I removed this template function, but left the template function TakesGeneric below to show that the automatic type does not require/give a warning. Is that alright?

Looks mostly good.

test/clang-tidy/abseil-duration-division.cpp
59

The check will still give warnings in the template instantiation, I think we need unless(isInTemplateInstantiation() matcher to filter them out.

deannagarcia added inline comments.
test/clang-tidy/abseil-duration-division.cpp
59

I added that and then put the test back to show that it now won't trigger a warning. Is that what you wanted?

hokein accepted this revision.Aug 14 2018, 7:51 AM

Thanks, looks good from my side. I will commit for you if other reviewers have no further comments.

This revision is now accepted and ready to land.Aug 14 2018, 7:51 AM
hokein closed this revision.Aug 17 2018, 8:22 AM

Committed in rL340038.

Somehow documentation file was not committed.

Somehow documentation file was not committed.

Oops, I forgot to git add to the doc file. arc patch somehow failed to apply this patch, I applied it manually. Added in rL340075. Thanks for pointing it out.