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.
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)
Please wrap return onto the next line. clang-format should do that automatically.
Please follow the LLVM naming convention (IsDuration in this case, maybe DurationExpr would be better too).
What about different kinds of casts, like C-Style casts?
Please follow the naming convention here as well -> Op or better OpCall or similar to have more telling names.
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?
I think potential instead of possible would sound better.
Please you 2 '``' signs for code-constructs in the .rst documentation.
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.
Maybe its better to use 3.0 in the assert to signify its an floating point value.
Please add examples with template-functions. What is the suggestion there?
template <class T> void DoGenericStuff(T t); DoGenericStuff(Duration(10) / Duration(1));
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
what about float? I guess it shuold behave identically?
What about other integer types like long. What about short, char?
Please add c-style casts as well. Even though they are not supposed to be used.
op still is lowercase.
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.
Thanks for contributing to clang-tidy!
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.
+1, we need to add more test cases.
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.
I linked our general documentation on absl::Duration, specifically the stuff on duration arithmetic. Is that what you wanted?
Does this link work or do you still want more?
The argumentCountIs should be redundant, not? Operator/ has always two operands and must be overloaded as an binary operator.
I dont understand the expr().bind, you can directly bind to the cxxOperatorCallExpr. That should be equivalent.
The common For the user-facing documentation see: <LinkToDoc> is missing here.
maybe call it DurationExpr since you have declared the variable as expr(...).
expr(IsDuration) seems have a duplicated expr, isn't hasArgument(0, IsDuration()) enough?
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.
I think we should ignore templates. The template function could be used by other types, fixing the template is not correct.
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?