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

[deannagarcia]

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.

[JonasToth]

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.

[JonasToth]

clang-tidy/abseil/DurationDivisionCheck.cpp

31

s/is_duration/IsDuration/ twice

38

op still is lowercase.

docs/ReleaseNotes.rst

59 ↗

(On Diff #159590)

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]

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]

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]

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

9

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

The sentence is incomplete.

[JonasToth]

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!

[JonasToth]

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.

[JonasToth]

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]

clang-tidy/abseil/DurationDivisionCheck.cpp

51

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

[JonasToth]

clang-tidy/abseil/DurationDivisionCheck.cpp

51

Leave as is :)

[hokein]

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]

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?

[hokein]

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]

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]

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

[hokein]

Committed in rL340038.

[Eugene.Zelenko]

Somehow documentation file was not committed.

[hokein]

In D50389#1204514, @Eugene.Zelenko wrote:

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.

clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.cpp - clang-tidy ----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationDivisionCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<DurationDivisionCheck>(
+        "abseil-duration-division");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }
 };
 
 // Register the AbseilModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
                                                     "Add Abseil checks.");

clang-tidy/abseil/DurationDivisionCheck.h

+//===--- DurationDivisionCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONDIVISIONCHECK_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONDIVISIONCHECK_H_
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+// Find potential incorrect uses of integer division of absl::Duration objects.
+class DurationDivisionCheck : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONDIVISIONCHECK_H_

clang-tidy/abseil/DurationDivisionCheck.cpp

+//===--- DurationDivisionCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DurationDivisionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+using namespace clang::ast_matchers;
+
+void DurationDivisionCheck::registerMatchers(MatchFinder *finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  const auto IsDuration =
+      expr(hasType(cxxRecordDecl(hasName("::absl::Duration"))));
+  finder->addMatcher(
+      implicitCastExpr(
+          hasSourceExpression(ignoringParenCasts(cxxOperatorCallExpr(
+              hasOverloadedOperatorName("/"), argumentCountIs(2),
+              hasArgument(0, expr(is_duration)),
+              hasArgument(1, expr(is_duration)), expr().bind("OpCall")))),
+          hasImplicitDestinationType(qualType(unless(isInteger()))),
+          unless(hasParent(cxxStaticCastExpr()))),
+      this);
+}
+
+void DurationDivisionCheck::check(const MatchFinder::MatchResult &result) {
+  const auto *op = result.Nodes.getNodeAs<CXXOperatorCallExpr>("OpCall");
+  diag(op->getOperatorLoc(),
+       "operator/ on absl::Duration objects performs integer division; "
+       "did you mean to use FDivDuration()?")
+      << FixItHint::CreateInsertion(op->getLocStart(), "absl::FDivDuration(")
+      << FixItHint::CreateReplacement(
+             SourceRange(op->getOperatorLoc(), op->getOperatorLoc()), ", ")
+      << FixItHint::CreateInsertion(
+             Lexer::getLocForEndOfToken(
+                 result.SourceManager->getSpellingLoc(op->getLocEnd()), 0,
+                 *result.SourceManager, result.Context->getLangOpts()),
+             ")");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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

+.. title:: clang-tidy - abseil-duration-division
+  
+abseil-duration-division
+========================
+
+``absl::Duration`` arithmetic works like it does with integers. That means that
+division of two ``absl::Duration`` objects returns an ``int64`` with any fractional
+component truncated toward 0.
+
+For example:
+
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ int64 sec1 = d / absl::Seconds(1);     // Truncates toward 0.
+ int64 sec2 = absl::ToInt64Seconds(d);  // Equivalent to division.
+ assert(sec1 == 3 && sec2 == 3);
+
+ double dsec = d / absl::Seconds(1);  // WRONG: Still truncates toward 0.
+ assert(dsec == 3.0);
+
+If you want floating-point division, you should use either the
+``absl::FDivDuration()`` function, or one of the unit conversion functions such
+as ``absl::ToDoubleSeconds()``. For example:
+    
+.. code-block:: c++
+
+ absl::Duration d = absl::Seconds(3.5);
+ double dsec1 = absl::FDivDuration(d, absl::Seconds(1));  // GOOD: No truncation.
+ double dsec2 = absl::ToDoubleSeconds(d);                 // GOOD: No truncation.
+ assert(dsec1 == 3.5 && dsec2 == 3.5);
+
+
+This check looks for uses of ``absl::Duration`` division that is done in a
+floating-point context, and recommends the use of a function that returns a
+floating-point value.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-duration-division
    abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen

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

+// RUN: %check_clang_tidy %s abseil-duration-division %t
+
+namespace absl {
+
+class Duration {};
+
+int operator/(Duration lhs, Duration rhs);
+
+double FDivDuration(Duration num, Duration den);
+
+}  // namespace absl
+
+void TakesInt(int);
+void TakesDouble(double);
+
+absl::Duration d;
+
+#define MACRO_EQ(x, y) (x == y)
+#define MACRO_DIVEQ(x,y,z) (x/y == z)
+#define CHECK(x) (x)
+
+void Positives() {
+  const double num = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: operator/ on Duration objects performs integer division; did you mean to use FDivDuration()? [abseil-duration-division]
+  // CHECK-FIXES: const double num = absl::FDivDuration(d, d);
+  if (MACRO_EQ(d/d, 0.0)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator/ on Duration objects
+  // CHECK-FIXES: if (MACRO_EQ(absl::FDivDuration(d, d), 0.0)) {}
+  if (CHECK(MACRO_EQ(d/d, 0.0))) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: operator/ on Duration objects
+  // CHECK-FIXES: if (CHECK(MACRO_EQ(absl::FDivDuration(d, d), 0.0))) {}
+
+  // This one generates a message, but no fix.
+  if (MACRO_DIVEQ(d, d, 0.0)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: operator/ on Duration objects
+  // CHECK-FIXES: if (MACRO_DIVEQ(d, d, 0.0)) {}
+}
+
+void Negatives() {
+  const int num = d/d;
+
+  // Explicit cast should disable the warning.
+  const double num_d = static_cast<double>(d/d);
+}