[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check
hokein added a comment. In https://reviews.llvm.org/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 https://reviews.llvm.org/rL340075. Thanks for pointing it out. 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] Abseil: integral division of Duration check
Eugene.Zelenko added a comment. Somehow documentation file was not committed. 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] Abseil: integral division of Duration check
hokein closed this revision. hokein added a comment. Committed in https://reviews.llvm.org/rL340038. 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] Abseil: integral division of Duration check
deannagarcia added inline comments.
Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+ // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration
objects
+ // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+ // absl::FDivDuration(t1, t2);}
hokein wrote:
> deannagarcia wrote:
> > hokein wrote:
> > > 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?
> The check will still give warnings in the template instantiation, I think we
> need `unless(isInTemplateInstantiation()` matcher to filter them out.
I added that and then put the test back to show that it now won't trigger a
warning. 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] Abseil: integral division of Duration check
deannagarcia updated this revision to Diff 160582.
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 TakesDouble(double);
+
+#define MACRO_EQ(x, y) (x == y)
+#define MACRO_DIVEQ(x,y,z) (x/y == z)
+#define CHECK(x) (x)
+
+void Positives() {
+ absl::Duration d;
+
+ 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));
+}
+
+void TakesInt(int);
+template
+void TakesGeneric(T);
+
+void Negatives() {
+ absl::Duration d;
+ 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);
+}
+
+template
+double DoubleDivision(T t1, T t2) {return t1/t2;}
+
+//This also won't trigger a warning
+void TemplateDivision() {
+ absl::Duration d;
+ DoubleDivision(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(1)); // GOOD: No truncation.
+ double dsec2 = absl::ToDoubleSeconds(d); // GOOD: No truncation.
+ assert(dsec1 == 3.5 &&
[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check
hokein added a comment.
Looks mostly good.
Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+ // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration
objects
+ // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+ // absl::FDivDuration(t1, t2);}
deannagarcia wrote:
> hokein wrote:
> > 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?
The check will still give warnings in the template instantiation, I think we
need `unless(isInTemplateInstantiation()` matcher to filter them out.
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] Abseil: integral division of Duration check
deannagarcia updated this revision to Diff 160338.
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,66 @@
+// 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 TakesDouble(double);
+
+#define MACRO_EQ(x, y) (x == y)
+#define MACRO_DIVEQ(x,y,z) (x/y == z)
+#define CHECK(x) (x)
+
+void Positives() {
+ absl::Duration d;
+
+ 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));
+}
+
+void TakesInt(int);
+template
+void TakesGeneric(T);
+
+void Negatives() {
+ absl::Duration d;
+ 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(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
[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check
deannagarcia updated this revision to Diff 160336.
deannagarcia marked 6 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,66 @@
+// 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);
+
+#define MACRO_EQ(x, y) (x == y)
+#define MACRO_DIVEQ(x,y,z) (x/y == z)
+#define CHECK(x) (x)
+
+void Positives() {
+ absl::Duration d;
+
+ 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));
+}
+
+void TakesDouble(double);
+template
+void TakesGeneric(T);
+
+void Negatives() {
+ absl::Duration d;
+ 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(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
[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check
deannagarcia added inline comments.
Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+ // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration
objects
+ // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+ // absl::FDivDuration(t1, t2);}
hokein wrote:
> 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?
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] Abseil: integral division of Duration check
hokein added inline comments.
Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:24
+
+ const auto IsDuration =
+ expr(hasType(cxxRecordDecl(hasName("::absl::Duration";
maybe call it `DurationExpr` since you have declared the variable as
`expr(...)`.
Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:30
+ hasOverloadedOperatorName("/"),
+ hasArgument(0, expr(IsDuration)),
+ hasArgument(1, expr(IsDuration))).bind("OpCall"))),
`expr(IsDuration)` seems have a duplicated `expr`, isn't `hasArgument(0,
IsDuration())` enough?
Comment at: test/clang-tidy/abseil-duration-division.cpp:18
+
+absl::Duration d;
+
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`.
Comment at: test/clang-tidy/abseil-duration-division.cpp:58
+ // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration
objects
+ // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return
+ // absl::FDivDuration(t1, t2);}
I think we should ignore templates. The template function could be used by
other types, fixing the template is not correct.
https://reviews.llvm.org/D50389
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
