[PATCH] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:50
+ *result.SourceManager, result.Context->getLangOpts()),
+ ")");
+}

deannagarcia wrote:
> JonasToth wrote:
> > This line looks odd, does it come from clang-format?
> I have run clang-format on this file and it's still formatted like this.
Leave as is :)


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:50
+ *result.SourceManager, result.Context->getLangOpts()),
+ ")");
+}

JonasToth wrote:
> This line looks odd, does it come from clang-format?
I have run clang-format on this file and it's still formatted like this.


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM with only the formatting question.

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




Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:50
+ *result.SourceManager, result.Context->getLangOpts()),
+ ")");
+}

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


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 160109.
deannagarcia marked 3 inline comments as done.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,75 @@
+// 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);
+template 
+void TakesGeneric(T);
+
+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_double = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:30: warning: operator/ on absl::Duration objects performs integer division; did you mean to use FDivDuration()? [abseil-duration-division]
+  // CHECK-FIXES: const double num_double = absl::FDivDuration(d, d);
+  const float num_float = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const float num_float = absl::FDivDuration(d, d);
+  const auto SomeVal = 1.0 + d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:31: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const auto SomeVal = 1.0 + absl::FDivDuration(d, d);
+  if (MACRO_EQ(d/d, 0.0)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator/ on absl::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 absl::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 absl::Duration objects
+  // CHECK-FIXES: if (MACRO_DIVEQ(d, d, 0.0)) {}
+ 
+  TakesDouble(d/d);
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: TakesDouble(absl::FDivDuration(d, d));
+}
+
+template 
+double DoubleDivision(T t1, T t2) {return t1/t2;}
+
+void AnotherPositive() {
+  DoubleDivision(d, d);
+  // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+  // absl::FDivDuration(t1, t2);}
+}
+
+void Negatives() {
+  const int num_int = d/d;
+  const long num_long = d/d;
+  const short num_short = d/d;
+  const char num_char = d/d;
+  const auto num_auto = d/d;
+  const auto SomeVal = 1 + d/d;
+
+  TakesInt(d/d);
+  TakesGeneric(d/d);
+  // Explicit cast should disable the warning.
+  const double num_cast1 = static_cast(d/d);
+  const double num_cast2 = (double)(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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. See `this link `_ for more information on arithmetic with ``absl::Duration``.
+
+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

[PATCH] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:29
+  hasSourceExpression(ignoringParenCasts(cxxOperatorCallExpr(
+  hasOverloadedOperatorName("/"), argumentCountIs(2),
+  hasArgument(0, expr(IsDuration)),

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



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:31
+  hasArgument(0, expr(IsDuration)),
+  hasArgument(1, expr(IsDuration)), expr().bind("OpCall",
+  hasImplicitDestinationType(qualType(unless(isInteger(,

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



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:19
+
+// Find potential incorrect uses of integer division of absl::Duration objects.
+class DurationDivisionCheck : public ClangTidyCheck {

The common `For the user-facing documentation see: ` 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.


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 160096.
deannagarcia marked 7 inline comments as done.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,75 @@
+// 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);
+template 
+void TakesGeneric(T);
+
+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_double = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:30: warning: operator/ on absl::Duration objects performs integer division; did you mean to use FDivDuration()? [abseil-duration-division]
+  // CHECK-FIXES: const double num_double = absl::FDivDuration(d, d);
+  const float num_float = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const float num_float = absl::FDivDuration(d, d);
+  const auto SomeVal = 1.0 + d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:31: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const auto SomeVal = 1.0 + absl::FDivDuration(d, d);
+  if (MACRO_EQ(d/d, 0.0)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator/ on absl::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 absl::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 absl::Duration objects
+  // CHECK-FIXES: if (MACRO_DIVEQ(d, d, 0.0)) {}
+ 
+  TakesDouble(d/d);
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: TakesDouble(absl::FDivDuration(d, d));
+}
+
+template 
+double DoubleDivision(T t1, T t2) {return t1/t2;}
+
+void AnotherPositive() {
+  DoubleDivision(d, d);
+  // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+  // absl::FDivDuration(t1, t2);}
+}
+
+void Negatives() {
+  const int num_int = d/d;
+  const long num_long = d/d;
+  const short num_short = d/d;
+  const char num_char = d/d;
+  const auto num_auto = d/d;
+  const auto SomeVal = 1 + d/d;
+
+  TakesInt(d/d);
+  TakesGeneric(d/d);
+  // Explicit cast should disable the warning.
+  const double num_cast1 = static_cast(d/d);
+  const double num_cast2 = (double)(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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. See `this link `_ for more information on arithmetic with ``absl::Duration``.
+
+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

[PATCH] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:32
+  hasImplicitDestinationType(qualType(unless(isInteger(,
+  unless(hasParent(cxxStaticCastExpr(,
+  this);

deannagarcia wrote:
> JonasToth wrote:
> > What about different kinds of casts, like C-Style casts?
> > Doesn't the `hasImplicitDestinationType` already remove the possibility for 
> > an cast as destination?
> 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.
Ok. I can not help there. Just leave as is :)



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:8
+division of two `absl::Duration` objects returns an `int64` with any fractional
+component truncated toward 0.
+

deannagarcia wrote:
> JonasToth wrote:
> > 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.
> Does this link work or do you still want more?
Link is enough!


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:8
+division of two ``absl::Duration`` objects returns an ``int64`` with any 
fractional
+component truncated toward 0. See `this link 
`_
 for more information on how ``absl::Duration`` arithmetic functions.
+

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

The sentence is incomplete.


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia marked 9 inline comments as done.
deannagarcia added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:32
+  hasImplicitDestinationType(qualType(unless(isInteger(,
+  unless(hasParent(cxxStaticCastExpr(,
+  this);

JonasToth wrote:
> What about different kinds of casts, like C-Style casts?
> Doesn't the `hasImplicitDestinationType` already remove the possibility for 
> an cast as destination?
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.



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:8
+division of two `absl::Duration` objects returns an `int64` with any fractional
+component truncated toward 0.
+

JonasToth wrote:
> 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.
Does this link work or do you still want more?



Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:3
+  
+abseil-duration-division
+

hokein wrote:
> 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.
I linked our general documentation on absl::Duration, specifically the stuff on 
duration arithmetic. Is that what you wanted?


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 160013.
Herald added a subscriber: mgorny.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,75 @@
+// 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);
+template 
+void TakesGeneric(T);
+
+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_double = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:30: warning: operator/ on absl::Duration objects performs integer division; did you mean to use FDivDuration()? [abseil-duration-division]
+  // CHECK-FIXES: const double num_double = absl::FDivDuration(d, d);
+  const float num_float = d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const float num_float = absl::FDivDuration(d, d);
+  const auto SomeVal = 1.0 + d/d;
+  // CHECK-MESSAGES: [[@LINE-1]]:31: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: const auto SomeVal = 1.0 + absl::FDivDuration(d, d);
+  if (MACRO_EQ(d/d, 0.0)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator/ on absl::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 absl::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 absl::Duration objects
+  // CHECK-FIXES: if (MACRO_DIVEQ(d, d, 0.0)) {}
+ 
+  TakesDouble(d/d);
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: TakesDouble(absl::FDivDuration(d, d));
+}
+
+template 
+double DoubleDivision(T t1, T t2) {return t1/t2;}
+
+void AnotherPositive() {
+  DoubleDivision(d, d);
+  // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration objects
+  // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+  // absl::FDivDuration(t1, t2);}
+}
+
+void Negatives() {
+  const int num_int = d/d;
+  const long num_long = d/d;
+  const short num_short = d/d;
+  const char num_char = d/d;
+  const auto num_auto = d/d;
+  const auto SomeVal = 1 + d/d;
+
+  TakesInt(d/d);
+  TakesGeneric(d/d);
+  // Explicit cast should disable the warning.
+  const double num_cast1 = static_cast(d/d);
+  const double num_cast2 = (double)(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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. See `this link `_ for more information on how ``absl::Duration`` arithmetic functions.
+
+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)

[PATCH] D50389: [clang-tidy] new check for Abseil

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for contributing to clang-tidy!




Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:3
+  
+abseil-duration-division
+

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.



Comment at: test/clang-tidy/abseil-duration-division.cpp:21
+#define CHECK(x) (x)
+
+void Positives() {

JonasToth wrote:
> 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
> ```
> 
+1, we need to add more test cases.


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:37
+void DurationDivisionCheck::check(const MatchFinder::MatchResult& result) {
+  const auto* op = result.Nodes.getNodeAs("op");
+  diag(op->getOperatorLoc(),

JonasToth wrote:
> Please follow the naming convention here as well -> `Op` or better `OpCall` 
> or similar to have more telling names.
`op` still is lowercase.



Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:30
+  hasOverloadedOperatorName("/"), argumentCountIs(2),
+  hasArgument(0, expr(is_duration)),
+  hasArgument(1, expr(is_duration)), expr().bind("OpCall",

s/is_duration/IsDuration/ twice



Comment at: docs/ReleaseNotes.rst:59
 --
+- New :doc:`abseil-duration-division
+  ` check.

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.


https://reviews.llvm.org/D50389



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 159590.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,44 @@
+// 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(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -56,6 +56,12 @@
 
 Improvements to clang-tidy
 --
+- New :doc:`abseil-duration-division
+  ` check.
+
+  Checks 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.
 
 The improvements are...
 
Index: clang-tidy/abseil/DurationDivisionCheck.h
===
--- clang-tidy/abseil/DurationDivisionCheck.h
+++ clang-tidy/abseil/DurationDivisionCheck.h
@@ -0,0 +1,31 @@
+//===--- 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_CL

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 159550.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,44 @@
+// 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(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -56,6 +56,12 @@
 
 Improvements to clang-tidy
 --
+- New :doc:``abseil-duration-division
+  `` check.
+
+  Checks 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.
 
 The improvements are...
 
Index: clang-tidy/abseil/DurationDivisionCheck.h
===
--- clang-tidy/abseil/DurationDivisionCheck.h
+++ clang-tidy/abseil/DurationDivisionCheck.h
@@ -0,0 +1,31 @@
+//===--- 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_

[PATCH] D50389: [clang-tidy] new check for Abseil

[Deanna Garcia via Phabricator via cfe-commits]

deannagarcia updated this revision to Diff 159537.

https://reviews.llvm.org/D50389

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/DurationDivisionCheck.cpp
  clang-tidy/abseil/DurationDivisionCheck.h
  docs/clang-tidy/checks/abseil-duration-division.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-division.cpp

Index: test/clang-tidy/abseil-duration-division.cpp
===
--- test/clang-tidy/abseil-duration-division.cpp
+++ test/clang-tidy/abseil-duration-division.cpp
@@ -0,0 +1,44 @@
+// 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(d/d);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-duration-division
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-duration-division.rst
===
--- docs/clang-tidy/checks/abseil-duration-division.rst
+++ docs/clang-tidy/checks/abseil-duration-division.rst
@@ -0,0 +1,36 @@
+.. 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.
Index: clang-tidy/abseil/DurationDivisionCheck.h
===
--- clang-tidy/abseil/DurationDivisionCheck.h
+++ clang-tidy/abseil/DurationDivisionCheck.h
@@ -0,0 +1,31 @@
+//===--- 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::MatchFi