[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349073: [clang-tidy] Add the abseil-duration-subtraction 
check (authored by hwright, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55245?vs=178039=178101#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
  clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-subtraction.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/abseil-duration-subtraction.cpp

Index: clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-subtraction.rst
===
--- clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-subtraction.rst
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-subtraction.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - abseil-duration-subtraction
+
+abseil-duration-subtraction
+===
+
+Checks for cases where subtraction should be performed in the
+``absl::Duration`` domain. When subtracting two values, and the first one is
+known to be a conversion from ``absl::Duration``, we can infer that the second
+should also be interpreted as an ``absl::Duration``, and make that inference
+explicit.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Subtraction in the double domain
+  double x;
+  absl::Duration d;
+  double result = absl::ToDoubleSeconds(d) - x;
+
+  // Suggestion - Subtraction in the absl::Duration domain instead
+  double result = absl::ToDoubleSeconds(d - absl::Seconds(x));
+
+
+  // Original - Subtraction of two Durations in the double domain
+  absl::Duration d1, d2;
+  double result = absl::ToDoubleSeconds(d1) - absl::ToDoubleSeconds(d2);
+
+  // Suggestion - Subtraction in the absl::Duration domain instead
+  double result = absl::ToDoubleSeconds(d1 - d2);
+
+Note: As with other ``clang-tidy`` checks, it is possible that multiple fixes
+may overlap (as in the case of nested expressions), so not all occurences can
+be transformed in one run. In particular, this may occur for nested subtraction
+expressions. Running ``clang-tidy`` multiple times will find and fix these
+overlaps.
Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
@@ -8,6 +8,7 @@
abseil-duration-division
abseil-duration-factory-float
abseil-duration-factory-scale
+   abseil-duration-subtraction
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst
===
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst
@@ -93,6 +93,12 @@
   Checks for cases where arguments to ``absl::Duration`` factory functions are
   scaled internally and could be changed to a different factory function.
 
+- New :doc:`abseil-duration-subtraction
+  ` check.
+
+  Checks for cases where subtraction should be performed in the
+  ``absl::Duration`` domain.
+
 - New :doc:`abseil-faster-strsplit-delimiter
   ` check.
 
Index: clang-tools-extra/trunk/test/clang-tidy/abseil-duration-subtraction.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/abseil-duration-subtraction.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1, d2;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Thanks for reviewing, I'll go ahead and commit.

I've also removed the hash specialization, since we moved to `llvm::IndexedMap` 
instead of `std::unordered_map` inside of `getInverseForScale`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM with the doc-nit.




Comment at: docs/clang-tidy/checks/abseil-duration-subtraction.rst:33
+Note: As with other ``clang-tidy`` checks, it is possible that multiple fixes
+may overlap (as in the case of nested expressions), so not all occurences can
+be transformed in one run. Running ``clang-tidy`` multiple times will fix these

I think it the doc should be explicit that this is the case for nesting the 
subtraction expression the issue can occur. Otherwise it might be a bit 
confusing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked an inline comment as done.
hwright added a comment.

I've updated the documentation, and the rebased to `master`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 178039.
hwright marked an inline comment as done.
hwright added a comment.

Rebase and update documentation


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1, d2;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // A nested occurance
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(5));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(5))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1)))
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -8,6 +8,7 @@
abseil-duration-division
abseil-duration-factory-float
abseil-duration-factory-scale
+   abseil-duration-subtraction
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: docs/clang-tidy/checks/abseil-duration-subtraction.rst
===
--- /dev/null
+++ 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Did you rebase the check onto the new master with your refactorings in?




Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > From this example starting:
> > > > > > 
> > > > > > - The RHS should be a nested expression with function calls, as the 
> > > > > > RHS is transformed to create the adversary example i mean in the 
> > > > > > transformation function above.
> > > > > > 
> > > > > > ```
> > > > > > absl::ToDoubleSeconds(d) - 
> > > > > > absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > > > > > absl::ToDoubleSeconds(d1));
> > > > > > ```
> > > > > > I think you need the proper conversion function, as the result of 
> > > > > > the expression is `double` and you need a `Duration`, right?
> > > > > > 
> > > > > > But in principle starting from this idea the transformation might 
> > > > > > break.
> > > > > I think there may be some confusion here (and that's entirely my 
> > > > > fault. :) )
> > > > > 
> > > > > We should never get this expression as input to the check, since it 
> > > > > doesn't compile (as you point out):
> > > > > ```
> > > > > absl::ToDoubleSeconds(d) - 
> > > > > absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > > > > absl::ToDoubleSeconds(d1));
> > > > > ```
> > > > > 
> > > > > Since `absl::ToDoubleSeconds` requires that its argument is an 
> > > > > `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
> > > > > absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this 
> > > > > as input.
> > > > > 
> > > > > There may be other expressions which could be input, but in practice 
> > > > > they don't really happen.  I've added a contrived example to the 
> > > > > tests, but at some point the tests get too complex and confuse the 
> > > > > fix matching infrastructure.
> > > > Your last sentence is the thing ;) Murphies Law will hit this check, 
> > > > too. In my opinion wrong transformations are very unfortunate and 
> > > > should be avoided if possible (in this case possible).
> > > > You can simply require that the expression of type double does not 
> > > > contain any duration subtraction calls.
> > > > 
> > > > This is even possible in the matcher-part of the check.
> > > I've written a test (which the testing infrastructure fails to handle 
> > > well, so I haven't included it in the diff), and it produces these 
> > > results:
> > > 
> > > ```
> > >//
> > >//
> > > -  x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5);
> > > +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) 
> > > - 5));
> > >//
> > >//
> > > -  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) 
> > > - 5));
> > > +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - 
> > > absl::Seconds(5;
> > > ```
> > > 
> > > Those results are correct.  There is a cosmetic issue of round tripping 
> > > through the `double` conversion in the 
> > > `absl::Seconds(absl::ToDoubleSeconds(...))` phrase, but untangling that 
> > > is 1) difficult (because of order of operations issues) and thus; 2) 
> > > probably the subject of a separate check.
> > > 
> > > This is still such a rare case (as in, we've not encountered it in 
> > > Google's codebase), that I'm not really concerned.  But if it's worth it 
> > > to explicitly exclude it from the traversal matcher, I can do that.
> > Can you say what the direct issue is? I would bet its the overlapping?
> > A note in the documentation would be ok from my side. When the conflicting 
> > transformations are tried to be applied clang-tidy does not crash but print 
> > a nice diagnostic and continue its life?
> Another example I've verified:
> ```
> -  x = absl::ToDoubleSeconds(d) - 
> absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
> +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 
> 5));
> ```
> 
> This a nested case, and while `clang-tidy` finds both of them, it only 
> applies the outer most one (presumably the one it finds first in its AST 
> traversal):
> ```
> note: this fix will not be applied because it overlaps with another fix
> ```
> 
> The new code can then be checked again to fix the internal instance.
> 
> It's not possible to express this case in a test because testing 
> infrastructure uses regular expressions, and the repeated strings in the test 
> expectation cause it to get a bit confused.
> 
> Given all the of the above, I'm unsure what content would go in the 
> documentation which is specific to this check.
Yes, that should be the 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked 2 inline comments as done.
hwright added inline comments.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > hwright wrote:
> > > > JonasToth wrote:
> > > > > From this example starting:
> > > > > 
> > > > > - The RHS should be a nested expression with function calls, as the 
> > > > > RHS is transformed to create the adversary example i mean in the 
> > > > > transformation function above.
> > > > > 
> > > > > ```
> > > > > absl::ToDoubleSeconds(d) - 
> > > > > absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > > > > absl::ToDoubleSeconds(d1));
> > > > > ```
> > > > > I think you need the proper conversion function, as the result of the 
> > > > > expression is `double` and you need a `Duration`, right?
> > > > > 
> > > > > But in principle starting from this idea the transformation might 
> > > > > break.
> > > > I think there may be some confusion here (and that's entirely my fault. 
> > > > :) )
> > > > 
> > > > We should never get this expression as input to the check, since it 
> > > > doesn't compile (as you point out):
> > > > ```
> > > > absl::ToDoubleSeconds(d) - 
> > > > absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > > > absl::ToDoubleSeconds(d1));
> > > > ```
> > > > 
> > > > Since `absl::ToDoubleSeconds` requires that its argument is an 
> > > > `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
> > > > absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as 
> > > > input.
> > > > 
> > > > There may be other expressions which could be input, but in practice 
> > > > they don't really happen.  I've added a contrived example to the tests, 
> > > > but at some point the tests get too complex and confuse the fix 
> > > > matching infrastructure.
> > > Your last sentence is the thing ;) Murphies Law will hit this check, too. 
> > > In my opinion wrong transformations are very unfortunate and should be 
> > > avoided if possible (in this case possible).
> > > You can simply require that the expression of type double does not 
> > > contain any duration subtraction calls.
> > > 
> > > This is even possible in the matcher-part of the check.
> > I've written a test (which the testing infrastructure fails to handle well, 
> > so I haven't included it in the diff), and it produces these results:
> > 
> > ```
> >//
> >//
> > -  x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5);
> > +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 
> > 5));
> >//
> >//
> > -  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 
> > 5));
> > +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - 
> > absl::Seconds(5;
> > ```
> > 
> > Those results are correct.  There is a cosmetic issue of round tripping 
> > through the `double` conversion in the 
> > `absl::Seconds(absl::ToDoubleSeconds(...))` phrase, but untangling that is 
> > 1) difficult (because of order of operations issues) and thus; 2) probably 
> > the subject of a separate check.
> > 
> > This is still such a rare case (as in, we've not encountered it in Google's 
> > codebase), that I'm not really concerned.  But if it's worth it to 
> > explicitly exclude it from the traversal matcher, I can do that.
> Can you say what the direct issue is? I would bet its the overlapping?
> A note in the documentation would be ok from my side. When the conflicting 
> transformations are tried to be applied clang-tidy does not crash but print a 
> nice diagnostic and continue its life?
Another example I've verified:
```
-  x = absl::ToDoubleSeconds(d) - 
absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
```

This a nested case, and while `clang-tidy` finds both of them, it only applies 
the outer most one (presumably the one it finds first in its AST traversal):
```
note: this fix will not be applied because it overlaps with another fix
```

The new code can then be checked again to fix the internal instance.

It's not possible to express this case in a test because testing infrastructure 
uses regular expressions, and the repeated strings in the test expectation 
cause it to get a bit confused.

Given all the of the above, I'm unsure what content would go in the 
documentation which is specific to this check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > From this example starting:
> > > > 
> > > > - The RHS should be a nested expression with function calls, as the RHS 
> > > > is transformed to create the adversary example i mean in the 
> > > > transformation function above.
> > > > 
> > > > ```
> > > > absl::ToDoubleSeconds(d) - 
> > > > absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > > > absl::ToDoubleSeconds(d1));
> > > > ```
> > > > I think you need the proper conversion function, as the result of the 
> > > > expression is `double` and you need a `Duration`, right?
> > > > 
> > > > But in principle starting from this idea the transformation might break.
> > > I think there may be some confusion here (and that's entirely my fault. 
> > > :) )
> > > 
> > > We should never get this expression as input to the check, since it 
> > > doesn't compile (as you point out):
> > > ```
> > > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) 
> > > - absl::ToDoubleSeconds(d1));
> > > ```
> > > 
> > > Since `absl::ToDoubleSeconds` requires that its argument is an 
> > > `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
> > > absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as 
> > > input.
> > > 
> > > There may be other expressions which could be input, but in practice they 
> > > don't really happen.  I've added a contrived example to the tests, but at 
> > > some point the tests get too complex and confuse the fix matching 
> > > infrastructure.
> > Your last sentence is the thing ;) Murphies Law will hit this check, too. 
> > In my opinion wrong transformations are very unfortunate and should be 
> > avoided if possible (in this case possible).
> > You can simply require that the expression of type double does not contain 
> > any duration subtraction calls.
> > 
> > This is even possible in the matcher-part of the check.
> I've written a test (which the testing infrastructure fails to handle well, 
> so I haven't included it in the diff), and it produces these results:
> 
> ```
>//
>//
> -  x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5);
> +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 
> 5));
>//
>//
> -  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 
> 5));
> +  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - 
> absl::Seconds(5;
> ```
> 
> Those results are correct.  There is a cosmetic issue of round tripping 
> through the `double` conversion in the 
> `absl::Seconds(absl::ToDoubleSeconds(...))` phrase, but untangling that is 1) 
> difficult (because of order of operations issues) and thus; 2) probably the 
> subject of a separate check.
> 
> This is still such a rare case (as in, we've not encountered it in Google's 
> codebase), that I'm not really concerned.  But if it's worth it to explicitly 
> exclude it from the traversal matcher, I can do that.
Can you say what the direct issue is? I would bet its the overlapping?
A note in the documentation would be ok from my side. When the conflicting 
transformations are tried to be applied clang-tidy does not crash but print a 
nice diagnostic and continue its life?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 177749.
hwright marked 6 inline comments as done.
hwright added a comment.

Rebase


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1, d2;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // A nested occurance
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(5));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(5))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1)))
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -8,6 +8,7 @@
abseil-duration-division
abseil-duration-factory-float
abseil-duration-factory-scale
+   abseil-duration-subtraction
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: docs/clang-tidy/checks/abseil-duration-subtraction.rst
===
--- /dev/null
+++ 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > From this example starting:
> > > 
> > > - The RHS should be a nested expression with function calls, as the RHS 
> > > is transformed to create the adversary example i mean in the 
> > > transformation function above.
> > > 
> > > ```
> > > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) 
> > > - absl::ToDoubleSeconds(d1));
> > > ```
> > > I think you need the proper conversion function, as the result of the 
> > > expression is `double` and you need a `Duration`, right?
> > > 
> > > But in principle starting from this idea the transformation might break.
> > I think there may be some confusion here (and that's entirely my fault. :) )
> > 
> > We should never get this expression as input to the check, since it doesn't 
> > compile (as you point out):
> > ```
> > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > absl::ToDoubleSeconds(d1));
> > ```
> > 
> > Since `absl::ToDoubleSeconds` requires that its argument is an 
> > `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
> > absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as 
> > input.
> > 
> > There may be other expressions which could be input, but in practice they 
> > don't really happen.  I've added a contrived example to the tests, but at 
> > some point the tests get too complex and confuse the fix matching 
> > infrastructure.
> Your last sentence is the thing ;) Murphies Law will hit this check, too. In 
> my opinion wrong transformations are very unfortunate and should be avoided 
> if possible (in this case possible).
> You can simply require that the expression of type double does not contain 
> any duration subtraction calls.
> 
> This is even possible in the matcher-part of the check.
I've written a test (which the testing infrastructure fails to handle well, so 
I haven't included it in the diff), and it produces these results:

```
   //
   //
-  x = absl::ToDoubleSeconds(d) - (absl::ToDoubleSeconds(d1) - 5);
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
   //
   //
-  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1) - 5));
+  x = absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1 - 
absl::Seconds(5;
```

Those results are correct.  There is a cosmetic issue of round tripping through 
the `double` conversion in the `absl::Seconds(absl::ToDoubleSeconds(...))` 
phrase, but untangling that is 1) difficult (because of order of operations 
issues) and thus; 2) probably the subject of a separate check.

This is still such a rare case (as in, we've not encountered it in Google's 
codebase), that I'm not really concerned.  But if it's worth it to explicitly 
exclude it from the traversal matcher, I can do that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+  getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst(
+  "e",

hwright wrote:
> JonasToth wrote:
> > In Principle the `Node` could have multiple expressions that are a call if 
> > there is nesting. 
> > The transformation is correct from what I see right now, but might result 
> > in the necessity of multiple passes for the check. (Is the addition version 
> > affected from that too?)
> > 
> > Given the recursive nature of the matcher you could construct a nesting 
> > with the inner part being a subtraction, too. The generated fixes would 
> > conflict and none of them would be applied. At least thats what I would 
> > expect right now. Please take a look at this issue.
> There isn't an imminent addition version at this point.
> 
> This matcher isn't recursive: it's just looking at the entire node to see if 
> it is a call to the inverse function.  If an inverse is embedded as part of a 
> deeper expression, it won't see it (e.g., there no `hasDescendant` in this 
> matcher).
Matchers are recursive. There will be a next match of the inner nodes below 
this node, just by AST traversal.



Comment at: clang-tidy/abseil/DurationSubtractionCheck.cpp:38
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID())
+return;

`Loc.isInvalid()` too. You can pass in macros from the command line that result 
in invalid sourcelocations.



Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {

hwright wrote:
> JonasToth wrote:
> > I think having the extraction of the common test-stuff into this header as 
> > one commit would be better. Would you prepare such a patch? I can commit 
> > for you. It probably makes sense if you ask for commit access 
> > (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as 
> > you wish.
> I can do this, but it might take a bit to get the commit bit turned on.
Maybe, I (or someone else) have no problem commiting for you.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

hwright wrote:
> JonasToth wrote:
> > From this example starting:
> > 
> > - The RHS should be a nested expression with function calls, as the RHS is 
> > transformed to create the adversary example i mean in the transformation 
> > function above.
> > 
> > ```
> > absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> > absl::ToDoubleSeconds(d1));
> > ```
> > I think you need the proper conversion function, as the result of the 
> > expression is `double` and you need a `Duration`, right?
> > 
> > But in principle starting from this idea the transformation might break.
> I think there may be some confusion here (and that's entirely my fault. :) )
> 
> We should never get this expression as input to the check, since it doesn't 
> compile (as you point out):
> ```
> absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> absl::ToDoubleSeconds(d1));
> ```
> 
> Since `absl::ToDoubleSeconds` requires that its argument is an 
> `absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
> absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as input.
> 
> There may be other expressions which could be input, but in practice they 
> don't really happen.  I've added a contrived example to the tests, but at 
> some point the tests get too complex and confuse the fix matching 
> infrastructure.
Your last sentence is the thing ;) Murphies Law will hit this check, too. In my 
opinion wrong transformations are very unfortunate and should be avoided if 
possible (in this case possible).
You can simply require that the expression of type double does not contain any 
duration subtraction calls.

This is even possible in the matcher-part of the check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 177590.
hwright marked 9 inline comments as done.
hwright added a comment.

Add tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-comparison.cpp
  test/clang-tidy/abseil-duration-factory-float.cpp
  test/clang-tidy/abseil-duration-factory-scale.cpp
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1, d2;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // A nested occurance
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(5));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(5))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1)))
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- test/clang-tidy/abseil-duration-factory-scale.cpp
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -1,32 +1,6 @@
-// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I %S/Inputs
 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {

JonasToth wrote:
> Are you using `argument_type`? Browser searching did only show one result.
This is required by `IndexedMap`, if I understand correctly.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+  unsigned operator()(DurationScale Scale) const {
+return static_cast(Scale);
+  }

JonasToth wrote:
> Why not `std::uint8_t` as its the underlying type for the `enum`?
This is required by `IndexedMap`, if I understand correctly.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+  getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst(
+  "e",

JonasToth wrote:
> In Principle the `Node` could have multiple expressions that are a call if 
> there is nesting. 
> The transformation is correct from what I see right now, but might result in 
> the necessity of multiple passes for the check. (Is the addition version 
> affected from that too?)
> 
> Given the recursive nature of the matcher you could construct a nesting with 
> the inner part being a subtraction, too. The generated fixes would conflict 
> and none of them would be applied. At least thats what I would expect right 
> now. Please take a look at this issue.
There isn't an imminent addition version at this point.

This matcher isn't recursive: it's just looking at the entire node to see if it 
is a call to the inverse function.  If an inverse is embedded as part of a 
deeper expression, it won't see it (e.g., there no `hasDescendant` in this 
matcher).



Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {

JonasToth wrote:
> I think having the extraction of the common test-stuff into this header as 
> one commit would be better. Would you prepare such a patch? I can commit for 
> you. It probably makes sense if you ask for commit access 
> (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as 
> you wish.
I can do this, but it might take a bit to get the commit bit turned on.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

JonasToth wrote:
> From this example starting:
> 
> - The RHS should be a nested expression with function calls, as the RHS is 
> transformed to create the adversary example i mean in the transformation 
> function above.
> 
> ```
> absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
> absl::ToDoubleSeconds(d1));
> ```
> I think you need the proper conversion function, as the result of the 
> expression is `double` and you need a `Duration`, right?
> 
> But in principle starting from this idea the transformation might break.
I think there may be some confusion here (and that's entirely my fault. :) )

We should never get this expression as input to the check, since it doesn't 
compile (as you point out):
```
absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
absl::ToDoubleSeconds(d1));
```

Since `absl::ToDoubleSeconds` requires that its argument is an 
`absl::Duration`, but the expression `absl::ToDoubleSeconds(d) - 
absl::ToDoubleSeconds(d1)` results in a `double`, we can't get this as input.

There may be other expressions which could be input, but in practice they don't 
really happen.  I've added a contrived example to the tests, but at some point 
the tests get too complex and confuse the fix matching infrastructure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {

Are you using `argument_type`? Browser searching did only show one result.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+  unsigned operator()(DurationScale Scale) const {
+return static_cast(Scale);
+  }

Why not `std::uint8_t` as its the underlying type for the `enum`?



Comment at: clang-tidy/abseil/DurationRewriter.cpp:73
+static llvm::Optional
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult ,
+DurationScale Scale, const Expr ) {

`maybe` in the name is redundant, as its return type is `Optional`



Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+  getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst(
+  "e",

In Principle the `Node` could have multiple expressions that are a call if 
there is nesting. 
The transformation is correct from what I see right now, but might result in 
the necessity of multiple passes for the check. (Is the addition version 
affected from that too?)

Given the recursive nature of the matcher you could construct a nesting with 
the inner part being a subtraction, too. The generated fixes would conflict and 
none of them would be applied. At least thats what I would expect right now. 
Please take a look at this issue.



Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {

I think having the extraction of the common test-stuff into this header as one 
commit would be better. Would you prepare such a patch? I can commit for you. 
It probably makes sense if you ask for commit access 
(https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as you 
wish.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

From this example starting:

- The RHS should be a nested expression with function calls, as the RHS is 
transformed to create the adversary example i mean in the transformation 
function above.

```
absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
absl::ToDoubleSeconds(d1));
```
I think you need the proper conversion function, as the result of the 
expression is `double` and you need a `Duration`, right?

But in principle starting from this idea the transformation might break.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:20-39
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+switch (Scale) {
+case DurationScale::Hours:
+  return 0;
+case DurationScale::Minutes:

lebedev.ri wrote:
> You should not need this if you change the `enum` instead.
The function is still required; the switch can be removed with a `static_cast`.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:66-77
+InverseMap[DurationScale::Hours] =
+std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+InverseMap[DurationScale::Minutes] =
+std::make_pair("::absl::ToDoubleMinutes", 
"::absl::ToInt64Minutes");
+InverseMap[DurationScale::Seconds] =
+std::make_pair("::absl::ToDoubleSeconds", 
"::absl::ToInt64Seconds");
+InverseMap[DurationScale::Milliseconds] = std::make_pair(

lebedev.ri wrote:
> I was thinking of more like
> ```
> for(std::pair e : std::initializer_list({
>{DurationScale::Hours, 
> std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}.
>}))
>   InverseMap[e.first] = e.second;
> ```
The compilable construction looks something like:
```
for (const auto  : {std::make_pair(DurationScale::Hours,
std::make_pair("::absl::ToDoubleHours",
   "::absl::ToInt64Hours"))})
```
Which is a bit more verbose than just assigning values to the map (and not any 
more efficient), so I've just left this bit as-is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 177509.
hwright marked 5 inline comments as done.
hwright added a comment.

Use `static_cast` instead of a `switch` for `IndexedMap` lookup.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-comparison.cpp
  test/clang-tidy/abseil-duration-factory-float.cpp
  test/clang-tidy/abseil-duration-factory-scale.cpp
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- test/clang-tidy/abseil-duration-factory-scale.cpp
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -1,32 +1,6 @@
-// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I %S/Inputs
 
-// Mimic the implementation of absl::Duration
-namespace absl {
-
-class Duration {};
-
-Duration Nanoseconds(long long);
-Duration Microseconds(long long);
-Duration Milliseconds(long long);
-Duration Seconds(long long);
-Duration Minutes(long long);
-Duration Hours(long long);
-
-#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
-  Duration NAME(float n); \
-  Duration NAME(double n);\
-  template\
-  Duration NAME(T n);
-

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:20-39
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+switch (Scale) {
+case DurationScale::Hours:
+  return 0;
+case DurationScale::Minutes:

You should not need this if you change the `enum` instead.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:66-77
+InverseMap[DurationScale::Hours] =
+std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+InverseMap[DurationScale::Minutes] =
+std::make_pair("::absl::ToDoubleMinutes", 
"::absl::ToInt64Minutes");
+InverseMap[DurationScale::Seconds] =
+std::make_pair("::absl::ToDoubleSeconds", 
"::absl::ToInt64Seconds");
+InverseMap[DurationScale::Milliseconds] = std::make_pair(

I was thinking of more like
```
for(std::pair e : std::initializer_list({
   {DurationScale::Hours, 
std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}.
   }))
  InverseMap[e.first] = e.second;
```



Comment at: clang-tidy/abseil/DurationRewriter.h:22-23
 /// Duration factory and conversion scales
 enum class DurationScale : std::int8_t {
   Hours,
   Minutes,

```
enum class DurationScale : std::int8_t {
  Hours = 0,
  Minutes,
...
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Reminder: I'll need somebody to submit this for me, since I don't have 
subversion access.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 177325.
hwright marked 7 inline comments as done.
hwright added a comment.

Use an `IndexedMap` instead of an `std::unordered_map`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-comparison.cpp
  test/clang-tidy/abseil-duration-factory-float.cpp
  test/clang-tidy/abseil-duration-factory-scale.cpp
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- test/clang-tidy/abseil-duration-factory-scale.cpp
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -1,32 +1,6 @@
-// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I %S/Inputs
 
-// Mimic the implementation of absl::Duration
-namespace absl {
-
-class Duration {};
-
-Duration Nanoseconds(long long);
-Duration Microseconds(long long);
-Duration Milliseconds(long long);
-Duration Seconds(long long);
-Duration Minutes(long long);
-Duration Hours(long long);
-
-#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
-  Duration NAME(float n); \
-  Duration NAME(double n);\
-  template\
-  Duration NAME(T n);
-

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:35
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map>

lebedev.ri wrote:
> hwright wrote:
> > lebedev.ri wrote:
> > > https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> > > > We never use hash_set and unordered_set because they are generally very 
> > > > expensive (each insertion requires a malloc) and very non-portable.
> > > 
> > > Since the key is an enum, [[ 
> > > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h | 
> > > `llvm/ADT/IndexedMap.h` ]] should be a much better fit.
> > It doesn't look like `IndexedMap` has a constructor which takes an 
> > initializer list.  Without it, this code get a bit more difficult to read, 
> > and I'd prefer to optimize for readability here.
> The manual still 'recommends' not to use them.
> Simple solution: immediately invoked lambda
> Better solution: try to add such constructor to `IndexedMap`.
In hopes of not making this too much of a yak shave, I've gone with the 
immediately invoked lambda.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:74
+ Node, *Result.Context))) {
+return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }

lebedev.ri wrote:
> hwright wrote:
> > lebedev.ri wrote:
> > > So you generate fix-it, and then immediately degrade it into a string. 
> > > Weird.
> > This doesn't generate a fix-it, it just fetches the text of the given node 
> > as a `StringRef` but we're returning a `string`, so we need to convert.
> > 
> > Is there a more canonical method I should use to fetch a node's text?
> I don't know the answer, but have you tried looking what 
> `tooling::fixit::getText()` does internally?
I have.  It calls `internal::getText`, which, from the namespace, I'm hesitant 
to call in this context.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:156
+llvm::Optional getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::DenseMap ScaleMap(
+  {{"ToDoubleHours", DurationScale::Hours},

lebedev.ri wrote:
> Are you very sure this shouldn't be [[ 
> https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h | 
> `StringMap` ]]?
Nope.  Thanks for the catch!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:35
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map>

hwright wrote:
> lebedev.ri wrote:
> > https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> > > We never use hash_set and unordered_set because they are generally very 
> > > expensive (each insertion requires a malloc) and very non-portable.
> > 
> > Since the key is an enum, [[ 
> > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h | 
> > `llvm/ADT/IndexedMap.h` ]] should be a much better fit.
> It doesn't look like `IndexedMap` has a constructor which takes an 
> initializer list.  Without it, this code get a bit more difficult to read, 
> and I'd prefer to optimize for readability here.
The manual still 'recommends' not to use them.
Simple solution: immediately invoked lambda
Better solution: try to add such constructor to `IndexedMap`.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:68
+  getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst(
+  "e", match(callExpr(callee(functionDecl(

hwright wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > `if (const auto *MaybeCallArg`
> > Early return?
> I'm not quite sure what you mean by `Early return?`  Are you suggesting that 
> the call to `selectFirst` should be pulled out of the `if` conditional, and 
> then the inverse checked to return `llvm::None` first? 
Ah, nevermind then.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:74
+ Node, *Result.Context))) {
+return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }

hwright wrote:
> lebedev.ri wrote:
> > So you generate fix-it, and then immediately degrade it into a string. 
> > Weird.
> This doesn't generate a fix-it, it just fetches the text of the given node as 
> a `StringRef` but we're returning a `string`, so we need to convert.
> 
> Is there a more canonical method I should use to fetch a node's text?
I don't know the answer, but have you tried looking what 
`tooling::fixit::getText()` does internally?



Comment at: clang-tidy/abseil/DurationRewriter.cpp:156
+llvm::Optional getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::DenseMap ScaleMap(
+  {{"ToDoubleHours", DurationScale::Hours},

Are you very sure this shouldn't be [[ 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h | `StringMap` 
]]?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:35
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map>

lebedev.ri wrote:
> https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> > We never use hash_set and unordered_set because they are generally very 
> > expensive (each insertion requires a malloc) and very non-portable.
> 
> Since the key is an enum, [[ 
> https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h | 
> `llvm/ADT/IndexedMap.h` ]] should be a much better fit.
It doesn't look like `IndexedMap` has a constructor which takes an initializer 
list.  Without it, this code get a bit more difficult to read, and I'd prefer 
to optimize for readability here.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:68
+  getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst(
+  "e", match(callExpr(callee(functionDecl(

lebedev.ri wrote:
> lebedev.ri wrote:
> > `if (const auto *MaybeCallArg`
> Early return?
I'm not quite sure what you mean by `Early return?`  Are you suggesting that 
the call to `selectFirst` should be pulled out of the `if` conditional, and 
then the inverse checked to return `llvm::None` first? 



Comment at: clang-tidy/abseil/DurationRewriter.cpp:74
+ Node, *Result.Context))) {
+return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }

lebedev.ri wrote:
> So you generate fix-it, and then immediately degrade it into a string. Weird.
This doesn't generate a fix-it, it just fetches the text of the given node as a 
`StringRef` but we're returning a `string`, so we need to convert.

Is there a more canonical method I should use to fetch a node's text?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 177266.
hwright marked 8 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-comparison.cpp
  test/clang-tidy/abseil-duration-factory-float.cpp
  test/clang-tidy/abseil-duration-factory-scale.cpp
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- test/clang-tidy/abseil-duration-factory-scale.cpp
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -1,32 +1,6 @@
-// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I %S/Inputs
 
-// Mimic the implementation of absl::Duration
-namespace absl {
-
-class Duration {};
-
-Duration Nanoseconds(long long);
-Duration Microseconds(long long);
-Duration Milliseconds(long long);
-Duration Seconds(long long);
-Duration Minutes(long long);
-Duration Hours(long long);
-
-#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
-  Duration NAME(float n); \
-  Duration NAME(double n);\
-  template\
-  Duration NAME(T n);
-
-GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
-GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please always upload all patches with the full context (`-U9`)




Comment at: clang-tidy/abseil/DurationRewriter.cpp:35
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map>

https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> We never use hash_set and unordered_set because they are generally very 
> expensive (each insertion requires a malloc) and very non-portable.

Since the key is an enum, [[ 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h | 
`llvm/ADT/IndexedMap.h` ]] should be a much better fit.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:68
+  getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst(
+  "e", match(callExpr(callee(functionDecl(

`if (const auto *MaybeCallArg`



Comment at: clang-tidy/abseil/DurationRewriter.cpp:68
+  getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst(
+  "e", match(callExpr(callee(functionDecl(

lebedev.ri wrote:
> `if (const auto *MaybeCallArg`
Early return?



Comment at: clang-tidy/abseil/DurationRewriter.cpp:74
+ Node, *Result.Context))) {
+return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }

So you generate fix-it, and then immediately degrade it into a string. Weird.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added subscribers: astrelni, ahedberg.
hokein added a comment.
This revision is now accepted and ready to land.

The check looks good from my side, except one nit.

> I assume I've got the right reviewers here, but I've also been sending a 
> bunch of stuff your way lately, so if I'm overwhelming review capacity, 
> please let me know.

I think in long run, it would be desired to have Abseil experts participate in 
reviews of abseil-related checks (upstream clang-tidy reviewers don't have much 
knowledge about Abseil). Abseil team members (@astrelni, @ahedberg) might be 
more interested in theses checks which could accelerate the review process.




Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:4
+// Mimic the implementation of absl::Duration
+namespace absl {
+

We have multiple implementations in other `abseil-duration-*` tests, I think we 
could pull them out (in `test/clang-tidy/Inputs/absl/time/time.h`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D55245#1320546 , @hwright wrote:

> Ping.
>
> I assume I've got the right reviewers here, but I've also been sending a 
> bunch of stuff your way lately, so if I'm overwhelming review capacity, 
> please let me know.


Hi hyrum, you have the right reviews. I do not have review capacity today, but 
at the end of the week. We usually ping after a few days (~1 week), e.g. for me 
its a hobby and I need to work :)
If you have more patches in the pipeline, you can already work on them/push 
them to review. It is easier to review a bit more in one piece and generates 
higher throughput too.

If other reviewers have time, go for it :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Ping.

I assume I've got the right reviewers here, but I've also been sending a bunch 
of stuff your way lately, so if I'm overwhelming review capacity, please let me 
know.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 176757.
hwright marked an inline comment as done.
hwright added a comment.

Fix double space.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,112 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+class Time{};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+  Duration NAME(float n); \
+  Duration NAME(double n);\
+  template\
+  Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+using int64_t = long long int;
+
+double ToDoubleHours(Duration d);
+double ToDoubleMinutes(Duration d);
+double ToDoubleSeconds(Duration d);
+double ToDoubleMilliseconds(Duration d);
+double ToDoubleMicroseconds(Duration d);
+double ToDoubleNanoseconds(Duration d);
+int64_t ToInt64Hours(Duration d);
+int64_t ToInt64Minutes(Duration d);
+int64_t ToInt64Seconds(Duration d);
+int64_t ToInt64Milliseconds(Duration d);
+int64_t ToInt64Microseconds(Duration d);
+int64_t ToInt64Nanoseconds(Duration d);
+
+// Relational Operators
+constexpr bool operator<(Duration lhs, Duration rhs);
+constexpr bool operator>(Duration lhs, Duration rhs);
+constexpr bool operator>=(Duration lhs, Duration rhs);
+constexpr bool operator<=(Duration lhs, Duration rhs);
+constexpr bool operator==(Duration lhs, Duration rhs);
+constexpr bool operator!=(Duration lhs, Duration rhs);
+
+// Additive Operators
+inline Time operator+(Time lhs, Duration rhs);
+inline Time operator+(Duration lhs, Time rhs);
+inline Time operator-(Time lhs, Duration rhs);
+inline Duration operator-(Time lhs, Time rhs);
+
+}  // namespace absl
+
+void f() {
+  double x;
+  absl::Duration d, d1;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: 

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-subtraction.rst:7
+Checks for cases where subtraction should be performed in the
+``absl::Duration`` domain.  When subtracting two values, and the first one is
+known to be a conversion from ``absl::Duration``, we can infer that the second

Please fix double space.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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