[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> `int8` ? Did you mean `int8_t` or am I missing somthing ?

Your right, but the solution I wrote did actually not work anyway.. I just 
specialized `std::hash<>` now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D54737#1317128 , @JonasToth wrote:

> I had to revert and recommitted in rCTE348169 
> . `std::unordered_map Something, ...>` does not work, as `std::hash` is not specialized for it. 
> This behaviour seems to work for some compilers, but some not. I had to 
> google myself a bit for the best solution, but now I specialized the 
> hash-function to `std::hash()` in `unordered_set` which worked for 
> me locally and was suggested. Lets see ;)


`int8` ? Did you mean `int8_t` or am I missing somthing ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I had to revert and recommitted in rCTE348169 
. `std::unordered_map` does not work, as `std::hash` is not specialized for it. This 
behaviour seems to work for some compilers, but some not. I had to google 
myself a bit for the best solution, but now I specialized the hash-function to 
`std::hash()` in `unordered_set` which worked for me locally and was 
suggested. Lets see ;)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

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

Changed prior to commit:
  https://reviews.llvm.org/D54737?vs=176156=176431#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737

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/DurationComparisonCheck.h
  clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp

Index: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"abseil-duration-comparison");
 CheckFactories.registerCheck(
 "abseil-duration-division");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
@@ -2,9 +2,11 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
+  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "DurationFactoryScaleCheck.h"
+#include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/FixIt.h"
@@ -18,20 +19,6 @@
 namespace tidy {
 namespace abseil {
 
-namespace {
-
-// Potential scales of our inputs.
-enum class DurationScale {
-  Hours,
-  Minutes,
-  Seconds,
-  Milliseconds,
-  Microseconds,
-  Nanoseconds,
-};
-
-} // namespace
-
 // Given the name of a duration factory function, return the appropriate
 // `DurationScale` for that factory.  If no factory can be found for
 // `FactoryName`, return `None`.
@@ -129,39 +116,14 @@
   return llvm::None;
 }
 
-// Given a `Scale`, return the appropriate factory function call for
-// constructing a `Duration` for that scale.
-static llvm::StringRef GetFactoryForScale(DurationScale Scale) {
-  switch (Scale) {
-  case DurationScale::Hours:
-return "absl::Hours";
-  case DurationScale::Minutes:
-return "absl::Minutes";
-  case DurationScale::Seconds:
-return "absl::Seconds";
-  case DurationScale::Milliseconds:
-return "absl::Milliseconds";
-  case DurationScale::Microseconds:
-return "absl::Microseconds";
-  case DurationScale::Nanoseconds:
-return "absl::Nanoseconds";
-  }
-  llvm_unreachable("unknown scaling factor");
-}
-
 void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
   callExpr(
-  callee(functionDecl(
- hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
-"::absl::Milliseconds", "::absl::Seconds",
-"::absl::Minutes", "::absl::Hours"))
- .bind("call_decl")),
+  callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
   hasArgument(
   0,
   ignoringImpCasts(anyOf(
-  

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54737#1314946 , @hwright wrote:

> Oh, and thanks for taking the time to review this. :)


No problem!
I will commit on Monday evening (Europe evening) if there are no further 
comments from other reviewers.
Thank you for the patch!


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Oh, and thanks for taking the time to review this. :)


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

@JonasToth reminder that you (or somebody else) will need to commit this for me.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 176156.
hwright marked 2 inline comments as done.
hwright added a comment.

Add additional test


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

https://reviews.llvm.org/D54737

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

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %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 d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) == x;
+  // 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

Only the test and your opinion on the `Optional` thing, If you want to keep it 
that way its fine.

LGTM afterwards :)




Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > The diagnostic is not printed if for some reason the fixit was not 
> > > > > > creatable. I think that the warning could still be emitted (`auto 
> > > > > > Diag = diag(...); if (Fix) Diag << Fixit::-...`)
> > > > > I'm not sure under which conditions you'd expect this to be an issue. 
> > > > >  Could you give me an example?
> > > > > 
> > > > > My expectation is that if we don't get a value back in `SimpleArg`, 
> > > > > we don't have anything to change, so there wouldn't be a warning to 
> > > > > emit.
> > > > I don't expect this to fail, failure there would mean a bug i guess. 
> > > > Having bugs is somewhat expected :)
> > > > And that would be our way to find the bug, because some user reports 
> > > > that there is no transformation done, that is my motivation behind that.
> > > > 
> > > > The warning itself should be correct, otherwise the matcher does not 
> > > > work, right? This would just be precaution.
> > > I guess what I'm saying is that I'm not sure what kind of warning we'd 
> > > give if we weren't able to offer a fix.  The optionality here means that 
> > > we found a result which was already as simple as we can make it, so 
> > > there's no reason to bother the user.
> > Lets say the result comes out of arithmetic and then there is a comparison 
> > (would be good test btw :)), the warning is still valueable, even if the 
> > transformation fails for some reason. The transformation could fail as 
> > well, because macros interfere or so. Past experience tells me, there are 
> > enough language constructs to break everything in weird combinations :)
> I can buy that, but I'm still having trouble seeing the specifics.  Do you 
> have a concrete example?
No i can't think of one right now. Its more a general argument, that the 
code-layout logically allows that only the transformation fails, even if the 
detection succeeds. No strong opinion, but preference :)



Comment at: test/clang-tidy/abseil-duration-comparison.cpp:148
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToInt64Hours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the 
duration domain [abseil-duration-comparison]

please add a test where the `int` part is result of a complex arithmetic 
expression.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 176103.
hwright added a comment.

Slightly simplify the fixit text


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

https://reviews.llvm.org/D54737

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

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %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 d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

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

Ping.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

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



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > hwright wrote:
> > > > JonasToth wrote:
> > > > > The diagnostic is not printed if for some reason the fixit was not 
> > > > > creatable. I think that the warning could still be emitted (`auto 
> > > > > Diag = diag(...); if (Fix) Diag << Fixit::-...`)
> > > > I'm not sure under which conditions you'd expect this to be an issue.  
> > > > Could you give me an example?
> > > > 
> > > > My expectation is that if we don't get a value back in `SimpleArg`, we 
> > > > don't have anything to change, so there wouldn't be a warning to emit.
> > > I don't expect this to fail, failure there would mean a bug i guess. 
> > > Having bugs is somewhat expected :)
> > > And that would be our way to find the bug, because some user reports that 
> > > there is no transformation done, that is my motivation behind that.
> > > 
> > > The warning itself should be correct, otherwise the matcher does not 
> > > work, right? This would just be precaution.
> > I guess what I'm saying is that I'm not sure what kind of warning we'd give 
> > if we weren't able to offer a fix.  The optionality here means that we 
> > found a result which was already as simple as we can make it, so there's no 
> > reason to bother the user.
> Lets say the result comes out of arithmetic and then there is a comparison 
> (would be good test btw :)), the warning is still valueable, even if the 
> transformation fails for some reason. The transformation could fail as well, 
> because macros interfere or so. Past experience tells me, there are enough 
> language constructs to break everything in weird combinations :)
I can buy that, but I'm still having trouble seeing the specifics.  Do you have 
a concrete example?


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > The diagnostic is not printed if for some reason the fixit was not 
> > > > creatable. I think that the warning could still be emitted (`auto Diag 
> > > > = diag(...); if (Fix) Diag << Fixit::-...`)
> > > I'm not sure under which conditions you'd expect this to be an issue.  
> > > Could you give me an example?
> > > 
> > > My expectation is that if we don't get a value back in `SimpleArg`, we 
> > > don't have anything to change, so there wouldn't be a warning to emit.
> > I don't expect this to fail, failure there would mean a bug i guess. Having 
> > bugs is somewhat expected :)
> > And that would be our way to find the bug, because some user reports that 
> > there is no transformation done, that is my motivation behind that.
> > 
> > The warning itself should be correct, otherwise the matcher does not work, 
> > right? This would just be precaution.
> I guess what I'm saying is that I'm not sure what kind of warning we'd give 
> if we weren't able to offer a fix.  The optionality here means that we found 
> a result which was already as simple as we can make it, so there's no reason 
> to bother the user.
Lets say the result comes out of arithmetic and then there is a comparison 
(would be good test btw :)), the warning is still valueable, even if the 
transformation fails for some reason. The transformation could fail as well, 
because macros interfere or so. Past experience tells me, there are enough 
language constructs to break everything in weird combinations :)


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

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



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23
 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html
 
 class DurationDivisionCheck : public ClangTidyCheck {

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > I think that blank line could be removed, and it seems the comment is not 
> > > ///, could you take a look at it too?
> > > Touching this file is probably better to do in another patch anyway.
> > Agreed.  I think this snuck into the patch; I'll remove it.
> > 
> > (It would be good to just `clang-format` everything in this directory in a 
> > separate patch.)
> > 
> > The comment issue with `///` seems to be a common problem; is 
> > `clang-tidy/add_new_check.py` not generating correct code?
> add_new_check does ///, maybe IDE settings or so removed these? Maybe someone 
> created everything manually, dunno.
> 
> Doing the clang-format is ok, doesn't need review either (but plz run the 
> test before committing to master).
I don't think I yet have the commit bit...so somebody else wouldn't need to 
directly commit. :)



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > The diagnostic is not printed if for some reason the fixit was not 
> > > creatable. I think that the warning could still be emitted (`auto Diag = 
> > > diag(...); if (Fix) Diag << Fixit::-...`)
> > I'm not sure under which conditions you'd expect this to be an issue.  
> > Could you give me an example?
> > 
> > My expectation is that if we don't get a value back in `SimpleArg`, we 
> > don't have anything to change, so there wouldn't be a warning to emit.
> I don't expect this to fail, failure there would mean a bug i guess. Having 
> bugs is somewhat expected :)
> And that would be our way to find the bug, because some user reports that 
> there is no transformation done, that is my motivation behind that.
> 
> The warning itself should be correct, otherwise the matcher does not work, 
> right? This would just be precaution.
I guess what I'm saying is that I'm not sure what kind of warning we'd give if 
we weren't able to offer a fix.  The optionality here means that we found a 
result which was already as simple as we can make it, so there's no reason to 
bother the user.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23
 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html
 
 class DurationDivisionCheck : public ClangTidyCheck {

hwright wrote:
> JonasToth wrote:
> > I think that blank line could be removed, and it seems the comment is not 
> > ///, could you take a look at it too?
> > Touching this file is probably better to do in another patch anyway.
> Agreed.  I think this snuck into the patch; I'll remove it.
> 
> (It would be good to just `clang-format` everything in this directory in a 
> separate patch.)
> 
> The comment issue with `///` seems to be a common problem; is 
> `clang-tidy/add_new_check.py` not generating correct code?
add_new_check does ///, maybe IDE settings or so removed these? Maybe someone 
created everything manually, dunno.

Doing the clang-format is ok, doesn't need review either (but plz run the test 
before committing to master).



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

hwright wrote:
> JonasToth wrote:
> > The diagnostic is not printed if for some reason the fixit was not 
> > creatable. I think that the warning could still be emitted (`auto Diag = 
> > diag(...); if (Fix) Diag << Fixit::-...`)
> I'm not sure under which conditions you'd expect this to be an issue.  Could 
> you give me an example?
> 
> My expectation is that if we don't get a value back in `SimpleArg`, we don't 
> have anything to change, so there wouldn't be a warning to emit.
I don't expect this to fail, failure there would mean a bug i guess. Having 
bugs is somewhat expected :)
And that would be our way to find the bug, because some user reports that there 
is no transformation done, that is my motivation behind that.

The warning itself should be correct, otherwise the matcher does not work, 
right? This would just be precaution.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:69
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.
+  auto InverseIter = InverseMap.find(Scale);

JonasToth wrote:
> non-existence? Not sure about english, but i thought english does it that way
https://www.merriam-webster.com/dictionary/nonexistence tells me the hyphen 
isn't required.



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23
 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html
 
 class DurationDivisionCheck : public ClangTidyCheck {

JonasToth wrote:
> I think that blank line could be removed, and it seems the comment is not 
> ///, could you take a look at it too?
> Touching this file is probably better to do in another patch anyway.
Agreed.  I think this snuck into the patch; I'll remove it.

(It would be good to just `clang-format` everything in this directory in a 
separate patch.)

The comment issue with `///` seems to be a common problem; is 
`clang-tidy/add_new_check.py` not generating correct code?



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

JonasToth wrote:
> The diagnostic is not printed if for some reason the fixit was not creatable. 
> I think that the warning could still be emitted (`auto Diag = diag(...); if 
> (Fix) Diag << Fixit::-...`)
I'm not sure under which conditions you'd expect this to be an issue.  Could 
you give me an example?

My expectation is that if we don't get a value back in `SimpleArg`, we don't 
have anything to change, so there wouldn't be a warning to emit.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 175735.
hwright marked 13 inline comments as done.

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

https://reviews.llvm.org/D54737

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

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %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 d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LG from my side, only the style nits left.
other reviewers are invited to take a look too :)




Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:22
+
+// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+// return its `DurationScale`, or `None` if a match is not found.

Please use triple / for the function comment, for doxygen and consistency with 
documentation



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:69
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.
+  auto InverseIter = InverseMap.find(Scale);

non-existence? Not sure about english, but i thought english does it that way



Comment at: clang-tidy/abseil/DurationComparisonCheck.h:20
+/// Prefer comparison in the absl::Duration domain instead of the numeric
+// domain.
+///

Missing /



Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23
 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html
 
 class DurationDivisionCheck : public ClangTidyCheck {

I think that blank line could be removed, and it seems the comment is not ///, 
could you take a look at it too?
Touching this file is probably better to do in another patch anyway.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

The diagnostic is not printed if for some reason the fixit was not creatable. I 
think that the warning could still be emitted (`auto Diag = diag(...); if (Fix) 
Diag << Fixit::-...`)



Comment at: clang-tidy/abseil/DurationRewriter.cpp:19
+
+// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
+static llvm::Optional

Comment



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

you can ellide the braces



Comment at: clang-tidy/abseil/DurationRewriter.cpp:85
+// Attempt to simplify a `Duration` factory call with a literal argument.
+if (llvm::Optional IntValue = truncateIfIntegral(*LitFloat)) 
{
+  return IntValue->toString(/*radix=*/10);

you can ellide the braces here



Comment at: clang-tidy/abseil/DurationRewriter.h:21
+
+// Duration factory and conversion scales
+enum class DurationScale : std::int8_t {

Same comment things in this file (///)



Comment at: clang-tidy/abseil/DurationRewriter.h:56
+// Possibly further simplify a duration factory function's argument, without
+// changing the scale of the factory function.  Return that simplification or
+// the text of the argument if no simplification is possible.

Double space


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

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

Anything else for me here?


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

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



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

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > aaron.ballman wrote:
> > > > > sammccall wrote:
> > > > > > hwright wrote:
> > > > > > > JonasToth wrote:
> > > > > > > > hwright wrote:
> > > > > > > > > JonasToth wrote:
> > > > > > > > > > I think this could be made a `DenseMap` with just the right 
> > > > > > > > > > amount of dense entries. 12 elements seems not too much to 
> > > > > > > > > > me.
> > > > > > > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > > > > > > `StringRef`(or `StringLiteral` making everything 
> > > > > > > > > > `constexpr` if possible)?
> > > > > > > > > > Is there some strange stuff with dangling pointers or other 
> > > > > > > > > > issues going on?
> > > > > > > > > Conceptually, this could easily be `constexpr`, but my 
> > > > > > > > > compiler doesn't seem to want to build something which is 
> > > > > > > > > called `static constexpr llvm::DenseMap > > > > > > > > DurationScale>`.  It's chief complaint is that such a type 
> > > > > > > > > has a non-trivial destructor. Am I using this correctly?
> > > > > > > > I honestly never tried to make a `constexpr DenseMap<>` but it 
> > > > > > > > makes sense it is not possible, as `DenseMap` is involved with 
> > > > > > > > dynamic memory after all, which is not possible with 
> > > > > > > > `constexpr` (yet). So you were my test-bunny ;)
> > > > > > > > 
> > > > > > > > I did reread the Data-structures section in the LLVM manual, i 
> > > > > > > > totally forgot about `StringMap` that maps strings to values.
> > > > > > > > `DenseMap` is good when mapping small values to each other, as 
> > > > > > > > we do here (`const char* -> int (whatever the enum deduces 
> > > > > > > > too)`), which would fit.
> > > > > > > > `StringMap` does allocations for the strings, which we don't 
> > > > > > > > need.
> > > > > > > > 
> > > > > > > > `constexpr` aside my bet is that `DenseMap` fits this case the 
> > > > > > > > better, because we can lay every thing out and never touch it 
> > > > > > > > again. Maybe someone else has better arguments for the decision 
> > > > > > > > :)
> > > > > > > > 
> > > > > > > > Soo: could you please try `static const 
> > > > > > > > llvm::DenseMap`? :)
> > > > > > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > > > > > `static const llvm::DenseMap` 
> > > > > > > works here. :)
> > > > > > Sorry for the drive-by...
> > > > > > This has a non-trivial destructor, so violates 
> > > > > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
> > > > > >  at least in spirit.
> > > > > > 
> > > > > > Best to leak it (`auto  = *new const 
> > > > > > llvm::DenseMap<...>(...)`) to avoid the destructor issues. The 
> > > > > > constructor issues are already handled by making it function-local.
> > > > > I do not think this violates the coding standard -- that's talking 
> > > > > mostly about global objects, not local objects, and is concerned 
> > > > > about startup performance and initialization orders. I don't see any 
> > > > > destructor ordering issues with this, so I do not think any of that 
> > > > > applies here and leaking would be less than ideal.
> > > > There are three main issues with global objects that the coding 
> > > > standard mentions:
> > > >  - performance when unused. Not relevant to function-local statics, 
> > > > only constructed when used
> > > >  - static initialization order fiasco (constructors). Not relevant to 
> > > > function-local statics, constructed in well-defined order
> > > >  - static initialization order fiasco (destructors). Just as relevant 
> > > > to function-local statics as to any other object!
> > > > 
> > > > That's why I say it violates the rule in spirit: the destructor is 
> > > > global. https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> > > > 
> > > > > I don't see any destructor ordering issues with this
> > > > The reason these constructs are disallowed in coding guidelines is that 
> > > > humans can't see the problems, and they manifest in non-local ways :-(
> > > > 
> > > > > leaking would be less than ideal
> > > > Why? The current code will (usually) destroy the object when the 
> > > > program exits. This is a waste of CPU, the OS will free the memory.
> > > > The reason these constructs are disallowed in coding guidelines is that 
> > > > humans can't see the problems, and they manifest in non-local ways :-(
> > > 
> > > Can you explain why you think this would have any non-local impact on 
> > > destruction? The DenseMap is local to the function and a pointer to it 
> > > never escapes 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > sammccall wrote:
> > > > > hwright wrote:
> > > > > > JonasToth wrote:
> > > > > > > hwright wrote:
> > > > > > > > JonasToth wrote:
> > > > > > > > > I think this could be made a `DenseMap` with just the right 
> > > > > > > > > amount of dense entries. 12 elements seems not too much to me.
> > > > > > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > > > > > `StringRef`(or `StringLiteral` making everything `constexpr` 
> > > > > > > > > if possible)?
> > > > > > > > > Is there some strange stuff with dangling pointers or other 
> > > > > > > > > issues going on?
> > > > > > > > Conceptually, this could easily be `constexpr`, but my compiler 
> > > > > > > > doesn't seem to want to build something which is called `static 
> > > > > > > > constexpr llvm::DenseMap`.  
> > > > > > > > It's chief complaint is that such a type has a non-trivial 
> > > > > > > > destructor. Am I using this correctly?
> > > > > > > I honestly never tried to make a `constexpr DenseMap<>` but it 
> > > > > > > makes sense it is not possible, as `DenseMap` is involved with 
> > > > > > > dynamic memory after all, which is not possible with `constexpr` 
> > > > > > > (yet). So you were my test-bunny ;)
> > > > > > > 
> > > > > > > I did reread the Data-structures section in the LLVM manual, i 
> > > > > > > totally forgot about `StringMap` that maps strings to values.
> > > > > > > `DenseMap` is good when mapping small values to each other, as we 
> > > > > > > do here (`const char* -> int (whatever the enum deduces too)`), 
> > > > > > > which would fit.
> > > > > > > `StringMap` does allocations for the strings, which we don't need.
> > > > > > > 
> > > > > > > `constexpr` aside my bet is that `DenseMap` fits this case the 
> > > > > > > better, because we can lay every thing out and never touch it 
> > > > > > > again. Maybe someone else has better arguments for the decision :)
> > > > > > > 
> > > > > > > Soo: could you please try `static const 
> > > > > > > llvm::DenseMap`? :)
> > > > > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > > > > `static const llvm::DenseMap` works 
> > > > > > here. :)
> > > > > Sorry for the drive-by...
> > > > > This has a non-trivial destructor, so violates 
> > > > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
> > > > >  at least in spirit.
> > > > > 
> > > > > Best to leak it (`auto  = *new const 
> > > > > llvm::DenseMap<...>(...)`) to avoid the destructor issues. The 
> > > > > constructor issues are already handled by making it function-local.
> > > > I do not think this violates the coding standard -- that's talking 
> > > > mostly about global objects, not local objects, and is concerned about 
> > > > startup performance and initialization orders. I don't see any 
> > > > destructor ordering issues with this, so I do not think any of that 
> > > > applies here and leaking would be less than ideal.
> > > There are three main issues with global objects that the coding standard 
> > > mentions:
> > >  - performance when unused. Not relevant to function-local statics, only 
> > > constructed when used
> > >  - static initialization order fiasco (constructors). Not relevant to 
> > > function-local statics, constructed in well-defined order
> > >  - static initialization order fiasco (destructors). Just as relevant to 
> > > function-local statics as to any other object!
> > > 
> > > That's why I say it violates the rule in spirit: the destructor is 
> > > global. https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> > > 
> > > > I don't see any destructor ordering issues with this
> > > The reason these constructs are disallowed in coding guidelines is that 
> > > humans can't see the problems, and they manifest in non-local ways :-(
> > > 
> > > > leaking would be less than ideal
> > > Why? The current code will (usually) destroy the object when the program 
> > > exits. This is a waste of CPU, the OS will free the memory.
> > > The reason these constructs are disallowed in coding guidelines is that 
> > > humans can't see the problems, and they manifest in non-local ways :-(
> > 
> > Can you explain why you think this would have any non-local impact on 
> > destruction? The DenseMap is local to the function and a pointer to it 
> > never escapes (so nothing can rely on it), and the data contained are 
> > StringRefs wrapping string literals and integer values.
> > 
> > > Why? The current code will (usually) destroy the object when the program 
> > > exits. This is a waste of CPU, the OS will free the memory.
> > 
> > Static 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.h:62
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher,
+ DurationConversionFunction) {

JonasToth wrote:
> I think you can even make this an `AST_MATCHER(FunctionDecl, 
> durationConversionFunction) { ... }`, or was there an issue with it? (`git 
> grep -n AST_MATCHER` in clang-tidy for other examples)
> With this, the wrapping with `functionDecl()` should not be necessary.
Nevermind, that was wrong. That would do 
`functionDecl(durationConversionFunction())`, sorry for noise.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.h:62
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher,
+ DurationConversionFunction) {

I think you can even make this an `AST_MATCHER(FunctionDecl, 
durationConversionFunction) { ... }`, or was there an issue with it? (`git grep 
-n AST_MATCHER` in clang-tidy for other examples)
With this, the wrapping with `functionDecl()` should not be necessary.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map>
+  InverseMap(

hwright wrote:
> hwright wrote:
> > JonasToth wrote:
> > > This variable is a little hard to read. Could you make a little 
> > > wrapper-struct instead of the `tuple` to make clear which element 
> > > represents what?
> > > Otherwise, why not `std::pair`?
> > > 
> > > - same `DenseMap` argument
> > > - same `StringRef`/`StringLiteral` instead `string` consideration
> > `std::pair` works here.
> > 
> > I'll defer the `DenseMap` and `StringRef`/`StringLiteral` changes until we 
> > determine if they are actually possible.
> ...but using `DenseMap` here doesn't work.  From the mountains of compiler 
> output I get when I try, it appears that `DenseMap` adds some constraints on 
> the key type, and an `enum class` doesn't meet those constraints.
> 
> fwiw, the errors are mostly of this form:
> ```
> /llvm/llvm/include/llvm/ADT/DenseMap.h:1277:45: error: ‘getEmptyKey’ is not a 
> member of ‘llvm::DenseMapInfo’
>  const KeyT Empty = KeyInfoT::getEmptyKey();
> ~^~
> /llvm/llvm/include/llvm/ADT/DenseMap.h:1278:53: error: ‘getTombstoneKey’ is 
> not a member of ‘llvm::DenseMapInfo’
>  const KeyT Tombstone = KeyInfoT::getTombstoneKey();
> ~^~
> /llvm/llvm/include/llvm/ADT/DenseMap.h:1280:44: error: ‘isEqual’ is not a 
> member of ‘llvm::DenseMapInfo’
>  while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
>~^~~
> /llvm/llvm/include/llvm/ADT/DenseMap.h:1281:44: error: ‘isEqual’ is not a 
> member of ‘llvm::DenseMapInfo’
>KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
> ```
in order to use `DenseMap` you need to specialize the `DenseMapInfo` for types, 
that are not supported yet. More in the llvm manual and by examples.
Storing the integer is no issue, but the `enum class` makes its a type on its 
own, same with the pair.

I believe its fine to leave it an `unordered_map`, even though it might be 
slower on access. You can certainly do the extra-mile.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



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

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > hwright wrote:
> > > > > > > JonasToth wrote:
> > > > > > > > I think this could be made a `DenseMap` with just the right 
> > > > > > > > amount of dense entries. 12 elements seems not too much to me.
> > > > > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > > > > `StringRef`(or `StringLiteral` making everything `constexpr` if 
> > > > > > > > possible)?
> > > > > > > > Is there some strange stuff with dangling pointers or other 
> > > > > > > > issues going on?
> > > > > > > Conceptually, this could easily be `constexpr`, but my compiler 
> > > > > > > doesn't seem to want to build something which is called `static 
> > > > > > > constexpr llvm::DenseMap`.  It's 
> > > > > > > chief complaint is that such a type has a non-trivial destructor. 
> > > > > > > Am I using this correctly?
> > > > > > I honestly never tried to make a `constexpr DenseMap<>` but it 
> > > > > > makes sense it is not possible, as `DenseMap` is involved with 
> > > > > > dynamic memory after all, which is not possible with `constexpr` 
> > > > > > (yet). So you were my test-bunny ;)
> > > > > > 
> > > > > > I did reread the Data-structures section in the LLVM manual, i 
> > > > > > totally forgot about `StringMap` that maps strings to values.
> > > > > > `DenseMap` is good when mapping small values to each other, as we 
> > > > > > do here (`const char* -> int (whatever the enum deduces too)`), 
> > > > > > which would fit.
> > > > > > `StringMap` does allocations for the strings, which we don't need.
> > > > > > 
> > > > > > `constexpr` aside my bet is that `DenseMap` fits this case the 
> > > > > > better, because we can lay every thing out and never touch it 
> > > > > > again. Maybe someone else has better arguments for the decision :)
> > > > > > 
> > > > > > Soo: could you please try `static const 
> > > > > > llvm::DenseMap`? :)
> > > > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > > > `static const llvm::DenseMap` works 
> > > > > here. :)
> > > > Sorry for the drive-by...
> > > > This has a non-trivial destructor, so violates 
> > > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
> > > >  at least in spirit.
> > > > 
> > > > Best to leak it (`auto  = *new const 
> > > > llvm::DenseMap<...>(...)`) to avoid the destructor issues. The 
> > > > constructor issues are already handled by making it function-local.
> > > I do not think this violates the coding standard -- that's talking mostly 
> > > about global objects, not local objects, and is concerned about startup 
> > > performance and initialization orders. I don't see any destructor 
> > > ordering issues with this, so I do not think any of that applies here and 
> > > leaking would be less than ideal.
> > There are three main issues with global objects that the coding standard 
> > mentions:
> >  - performance when unused. Not relevant to function-local statics, only 
> > constructed when used
> >  - static initialization order fiasco (constructors). Not relevant to 
> > function-local statics, constructed in well-defined order
> >  - static initialization order fiasco (destructors). Just as relevant to 
> > function-local statics as to any other object!
> > 
> > That's why I say it violates the rule in spirit: the destructor is global. 
> > https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> > 
> > > I don't see any destructor ordering issues with this
> > The reason these constructs are disallowed in coding guidelines is that 
> > humans can't see the problems, and they manifest in non-local ways :-(
> > 
> > > leaking would be less than ideal
> > Why? The current code will (usually) destroy the object when the program 
> > exits. This is a waste of CPU, the OS will free the memory.
> > The reason these constructs are disallowed in coding guidelines is that 
> > humans can't see the problems, and they manifest in non-local ways :-(
> 
> Can you explain why you think this would have any non-local impact on 
> destruction? The DenseMap is local to the function and a pointer to it never 
> escapes (so nothing can rely on it), and the data contained are StringRefs 
> wrapping string literals and integer values.
> 
> > Why? The current code will (usually) destroy the object when the program 
> > exits. This is a waste of CPU, the OS will free the memory.
> 
> Static analysis tools will start barking about memory leaks which then adds 
> noise to the output, hiding real issues.
> Can you explain why you think this would have any non-local impact on 
> 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > hwright wrote:
> > > > JonasToth wrote:
> > > > > hwright wrote:
> > > > > > JonasToth wrote:
> > > > > > > I think this could be made a `DenseMap` with just the right 
> > > > > > > amount of dense entries. 12 elements seems not too much to me.
> > > > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > > > `StringRef`(or `StringLiteral` making everything `constexpr` if 
> > > > > > > possible)?
> > > > > > > Is there some strange stuff with dangling pointers or other 
> > > > > > > issues going on?
> > > > > > Conceptually, this could easily be `constexpr`, but my compiler 
> > > > > > doesn't seem to want to build something which is called `static 
> > > > > > constexpr llvm::DenseMap`.  It's 
> > > > > > chief complaint is that such a type has a non-trivial destructor. 
> > > > > > Am I using this correctly?
> > > > > I honestly never tried to make a `constexpr DenseMap<>` but it makes 
> > > > > sense it is not possible, as `DenseMap` is involved with dynamic 
> > > > > memory after all, which is not possible with `constexpr` (yet). So 
> > > > > you were my test-bunny ;)
> > > > > 
> > > > > I did reread the Data-structures section in the LLVM manual, i 
> > > > > totally forgot about `StringMap` that maps strings to values.
> > > > > `DenseMap` is good when mapping small values to each other, as we do 
> > > > > here (`const char* -> int (whatever the enum deduces too)`), which 
> > > > > would fit.
> > > > > `StringMap` does allocations for the strings, which we don't need.
> > > > > 
> > > > > `constexpr` aside my bet is that `DenseMap` fits this case the 
> > > > > better, because we can lay every thing out and never touch it again. 
> > > > > Maybe someone else has better arguments for the decision :)
> > > > > 
> > > > > Soo: could you please try `static const 
> > > > > llvm::DenseMap`? :)
> > > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > > `static const llvm::DenseMap` works 
> > > > here. :)
> > > Sorry for the drive-by...
> > > This has a non-trivial destructor, so violates 
> > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors 
> > > at least in spirit.
> > > 
> > > Best to leak it (`auto  = *new const llvm::DenseMap<...>(...)`) 
> > > to avoid the destructor issues. The constructor issues are already 
> > > handled by making it function-local.
> > I do not think this violates the coding standard -- that's talking mostly 
> > about global objects, not local objects, and is concerned about startup 
> > performance and initialization orders. I don't see any destructor ordering 
> > issues with this, so I do not think any of that applies here and leaking 
> > would be less than ideal.
> There are three main issues with global objects that the coding standard 
> mentions:
>  - performance when unused. Not relevant to function-local statics, only 
> constructed when used
>  - static initialization order fiasco (constructors). Not relevant to 
> function-local statics, constructed in well-defined order
>  - static initialization order fiasco (destructors). Just as relevant to 
> function-local statics as to any other object!
> 
> That's why I say it violates the rule in spirit: the destructor is global. 
> https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> 
> > I don't see any destructor ordering issues with this
> The reason these constructs are disallowed in coding guidelines is that 
> humans can't see the problems, and they manifest in non-local ways :-(
> 
> > leaking would be less than ideal
> Why? The current code will (usually) destroy the object when the program 
> exits. This is a waste of CPU, the OS will free the memory.
> The reason these constructs are disallowed in coding guidelines is that 
> humans can't see the problems, and they manifest in non-local ways :-(

Can you explain why you think this would have any non-local impact on 
destruction? The DenseMap is local to the function and a pointer to it never 
escapes (so nothing can rely on it), and the data contained are StringRefs 
wrapping string literals and integer values.

> Why? The current code will (usually) destroy the object when the program 
> exits. This is a waste of CPU, the OS will free the memory.

Static analysis tools will start barking about memory leaks which then adds 
noise to the output, hiding real issues.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



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

aaron.ballman wrote:
> sammccall wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > I think this could be made a `DenseMap` with just the right amount 
> > > > > > of dense entries. 12 elements seems not too much to me.
> > > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > > `StringRef`(or `StringLiteral` making everything `constexpr` if 
> > > > > > possible)?
> > > > > > Is there some strange stuff with dangling pointers or other issues 
> > > > > > going on?
> > > > > Conceptually, this could easily be `constexpr`, but my compiler 
> > > > > doesn't seem to want to build something which is called `static 
> > > > > constexpr llvm::DenseMap`.  It's 
> > > > > chief complaint is that such a type has a non-trivial destructor. Am 
> > > > > I using this correctly?
> > > > I honestly never tried to make a `constexpr DenseMap<>` but it makes 
> > > > sense it is not possible, as `DenseMap` is involved with dynamic memory 
> > > > after all, which is not possible with `constexpr` (yet). So you were my 
> > > > test-bunny ;)
> > > > 
> > > > I did reread the Data-structures section in the LLVM manual, i totally 
> > > > forgot about `StringMap` that maps strings to values.
> > > > `DenseMap` is good when mapping small values to each other, as we do 
> > > > here (`const char* -> int (whatever the enum deduces too)`), which 
> > > > would fit.
> > > > `StringMap` does allocations for the strings, which we don't need.
> > > > 
> > > > `constexpr` aside my bet is that `DenseMap` fits this case the better, 
> > > > because we can lay every thing out and never touch it again. Maybe 
> > > > someone else has better arguments for the decision :)
> > > > 
> > > > Soo: could you please try `static const 
> > > > llvm::DenseMap`? :)
> > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > `static const llvm::DenseMap` works here. 
> > > :)
> > Sorry for the drive-by...
> > This has a non-trivial destructor, so violates 
> > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors 
> > at least in spirit.
> > 
> > Best to leak it (`auto  = *new const llvm::DenseMap<...>(...)`) to 
> > avoid the destructor issues. The constructor issues are already handled by 
> > making it function-local.
> I do not think this violates the coding standard -- that's talking mostly 
> about global objects, not local objects, and is concerned about startup 
> performance and initialization orders. I don't see any destructor ordering 
> issues with this, so I do not think any of that applies here and leaking 
> would be less than ideal.
There are three main issues with global objects that the coding standard 
mentions:
 - performance when unused. Not relevant to function-local statics, only 
constructed when used
 - static initialization order fiasco (constructors). Not relevant to 
function-local statics, constructed in well-defined order
 - static initialization order fiasco (destructors). Just as relevant to 
function-local statics as to any other object!

That's why I say it violates the rule in spirit: the destructor is global. 
https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2

> I don't see any destructor ordering issues with this
The reason these constructs are disallowed in coding guidelines is that humans 
can't see the problems, and they manifest in non-local ways :-(

> leaking would be less than ideal
Why? The current code will (usually) destroy the object when the program exits. 
This is a waste of CPU, the OS will free the memory.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

sammccall wrote:
> hwright wrote:
> > JonasToth wrote:
> > > hwright wrote:
> > > > JonasToth wrote:
> > > > > I think this could be made a `DenseMap` with just the right amount of 
> > > > > dense entries. 12 elements seems not too much to me.
> > > > > Does the key-type need to be `std::string`, or could it be 
> > > > > `StringRef`(or `StringLiteral` making everything `constexpr` if 
> > > > > possible)?
> > > > > Is there some strange stuff with dangling pointers or other issues 
> > > > > going on?
> > > > Conceptually, this could easily be `constexpr`, but my compiler doesn't 
> > > > seem to want to build something which is called `static constexpr 
> > > > llvm::DenseMap`.  It's chief complaint 
> > > > is that such a type has a non-trivial destructor. Am I using this 
> > > > correctly?
> > > I honestly never tried to make a `constexpr DenseMap<>` but it makes 
> > > sense it is not possible, as `DenseMap` is involved with dynamic memory 
> > > after all, which is not possible with `constexpr` (yet). So you were my 
> > > test-bunny ;)
> > > 
> > > I did reread the Data-structures section in the LLVM manual, i totally 
> > > forgot about `StringMap` that maps strings to values.
> > > `DenseMap` is good when mapping small values to each other, as we do here 
> > > (`const char* -> int (whatever the enum deduces too)`), which would fit.
> > > `StringMap` does allocations for the strings, which we don't need.
> > > 
> > > `constexpr` aside my bet is that `DenseMap` fits this case the better, 
> > > because we can lay every thing out and never touch it again. Maybe 
> > > someone else has better arguments for the decision :)
> > > 
> > > Soo: could you please try `static const 
> > > llvm::DenseMap`? :)
> > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > `static const llvm::DenseMap` works here. :)
> Sorry for the drive-by...
> This has a non-trivial destructor, so violates 
> https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors at 
> least in spirit.
> 
> Best to leak it (`auto  = *new const llvm::DenseMap<...>(...)`) to 
> avoid the destructor issues. The constructor issues are already handled by 
> making it function-local.
I do not think this violates the coding standard -- that's talking mostly about 
global objects, not local objects, and is concerned about startup performance 
and initialization orders. I don't see any destructor ordering issues with 
this, so I do not think any of that applies here and leaking would be less than 
ideal.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



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

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > I think this could be made a `DenseMap` with just the right amount of 
> > > > dense entries. 12 elements seems not too much to me.
> > > > Does the key-type need to be `std::string`, or could it be 
> > > > `StringRef`(or `StringLiteral` making everything `constexpr` if 
> > > > possible)?
> > > > Is there some strange stuff with dangling pointers or other issues 
> > > > going on?
> > > Conceptually, this could easily be `constexpr`, but my compiler doesn't 
> > > seem to want to build something which is called `static constexpr 
> > > llvm::DenseMap`.  It's chief complaint is 
> > > that such a type has a non-trivial destructor. Am I using this correctly?
> > I honestly never tried to make a `constexpr DenseMap<>` but it makes sense 
> > it is not possible, as `DenseMap` is involved with dynamic memory after 
> > all, which is not possible with `constexpr` (yet). So you were my 
> > test-bunny ;)
> > 
> > I did reread the Data-structures section in the LLVM manual, i totally 
> > forgot about `StringMap` that maps strings to values.
> > `DenseMap` is good when mapping small values to each other, as we do here 
> > (`const char* -> int (whatever the enum deduces too)`), which would fit.
> > `StringMap` does allocations for the strings, which we don't need.
> > 
> > `constexpr` aside my bet is that `DenseMap` fits this case the better, 
> > because we can lay every thing out and never touch it again. Maybe someone 
> > else has better arguments for the decision :)
> > 
> > Soo: could you please try `static const llvm::DenseMap > DurationScale>`? :)
> > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> `static const llvm::DenseMap` works here. :)
Sorry for the drive-by...
This has a non-trivial destructor, so violates 
https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors at 
least in spirit.

Best to leak it (`auto  = *new const llvm::DenseMap<...>(...)`) to 
avoid the destructor issues. The constructor issues are already handled by 
making it function-local.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



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

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > I think this could be made a `DenseMap` with just the right amount of 
> > > dense entries. 12 elements seems not too much to me.
> > > Does the key-type need to be `std::string`, or could it be `StringRef`(or 
> > > `StringLiteral` making everything `constexpr` if possible)?
> > > Is there some strange stuff with dangling pointers or other issues going 
> > > on?
> > Conceptually, this could easily be `constexpr`, but my compiler doesn't 
> > seem to want to build something which is called `static constexpr 
> > llvm::DenseMap`.  It's chief complaint is 
> > that such a type has a non-trivial destructor. Am I using this correctly?
> I honestly never tried to make a `constexpr DenseMap<>` but it makes sense it 
> is not possible, as `DenseMap` is involved with dynamic memory after all, 
> which is not possible with `constexpr` (yet). So you were my test-bunny ;)
> 
> I did reread the Data-structures section in the LLVM manual, i totally forgot 
> about `StringMap` that maps strings to values.
> `DenseMap` is good when mapping small values to each other, as we do here 
> (`const char* -> int (whatever the enum deduces too)`), which would fit.
> `StringMap` does allocations for the strings, which we don't need.
> 
> `constexpr` aside my bet is that `DenseMap` fits this case the better, 
> because we can lay every thing out and never touch it again. Maybe someone 
> else has better arguments for the decision :)
> 
> Soo: could you please try `static const llvm::DenseMap DurationScale>`? :)
> (should work in theory: https://godbolt.org/z/Qo7Nv4)
`static const llvm::DenseMap` works here. :)



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map>
+  InverseMap(

hwright wrote:
> JonasToth wrote:
> > This variable is a little hard to read. Could you make a little 
> > wrapper-struct instead of the `tuple` to make clear which element 
> > represents what?
> > Otherwise, why not `std::pair`?
> > 
> > - same `DenseMap` argument
> > - same `StringRef`/`StringLiteral` instead `string` consideration
> `std::pair` works here.
> 
> I'll defer the `DenseMap` and `StringRef`/`StringLiteral` changes until we 
> determine if they are actually possible.
...but using `DenseMap` here doesn't work.  From the mountains of compiler 
output I get when I try, it appears that `DenseMap` adds some constraints on 
the key type, and an `enum class` doesn't meet those constraints.

fwiw, the errors are mostly of this form:
```
/llvm/llvm/include/llvm/ADT/DenseMap.h:1277:45: error: ‘getEmptyKey’ is not a 
member of ‘llvm::DenseMapInfo’
 const KeyT Empty = KeyInfoT::getEmptyKey();
~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1278:53: error: ‘getTombstoneKey’ is not 
a member of ‘llvm::DenseMapInfo’
 const KeyT Tombstone = KeyInfoT::getTombstoneKey();
~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1280:44: error: ‘isEqual’ is not a 
member of ‘llvm::DenseMapInfo’
 while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
   ~^~~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1281:44: error: ‘isEqual’ is not a 
member of ‘llvm::DenseMapInfo’
   KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
```



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+  hasAnyName(
+  "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+  "::absl::ToDoubleSeconds", 
"::absl::ToDoubleMilliseconds",

JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > the list here is somewhat duplicated with the static members in the 
> > > functions at the top. it would be best to merge them.
> > > Not sure on how much `constexpr` is supported by the llvm-datastructures, 
> > > but a constexpr `DenseMap.keys()` would be nice. Did you try something 
> > > along this line?
> > I haven't tried that exact formulation, but given the above issue with 
> > `DenseMap`, I'm not sure it will work.  Happy to try once we get it ironed 
> > out.
> > 
> > Another thing I've thought about is factoring the `functionDecl` matcher 
> > into a separate function, because I expect it to be reused.  I haven't been 
> > able to deduce what type that function would return.
> the type is probably `clang::ast_matchers::internal::Matcher`. 
> Not sure what the `bind` does with the type though.
> 
> If that is not the case you can make a nice error with `int Bad = 
> 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 175476.
hwright marked 10 inline comments as done.

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

https://reviews.llvm.org/D54737

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationComparisonCheck.h
  clang-tidy/abseil/DurationDivisionCheck.h
  clang-tidy/abseil/DurationFactoryFloatCheck.cpp
  clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-comparison.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-comparison.cpp

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %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 d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



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

hwright wrote:
> JonasToth wrote:
> > I think this could be made a `DenseMap` with just the right amount of dense 
> > entries. 12 elements seems not too much to me.
> > Does the key-type need to be `std::string`, or could it be `StringRef`(or 
> > `StringLiteral` making everything `constexpr` if possible)?
> > Is there some strange stuff with dangling pointers or other issues going on?
> Conceptually, this could easily be `constexpr`, but my compiler doesn't seem 
> to want to build something which is called `static constexpr 
> llvm::DenseMap`.  It's chief complaint is 
> that such a type has a non-trivial destructor. Am I using this correctly?
I honestly never tried to make a `constexpr DenseMap<>` but it makes sense it 
is not possible, as `DenseMap` is involved with dynamic memory after all, which 
is not possible with `constexpr` (yet). So you were my test-bunny ;)

I did reread the Data-structures section in the LLVM manual, i totally forgot 
about `StringMap` that maps strings to values.
`DenseMap` is good when mapping small values to each other, as we do here 
(`const char* -> int (whatever the enum deduces too)`), which would fit.
`StringMap` does allocations for the strings, which we don't need.

`constexpr` aside my bet is that `DenseMap` fits this case the better, because 
we can lay every thing out and never touch it again. Maybe someone else has 
better arguments for the decision :)

Soo: could you please try `static const llvm::DenseMap`? :)
(should work in theory: https://godbolt.org/z/Qo7Nv4)



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+  hasAnyName(
+  "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+  "::absl::ToDoubleSeconds", 
"::absl::ToDoubleMilliseconds",

hwright wrote:
> JonasToth wrote:
> > the list here is somewhat duplicated with the static members in the 
> > functions at the top. it would be best to merge them.
> > Not sure on how much `constexpr` is supported by the llvm-datastructures, 
> > but a constexpr `DenseMap.keys()` would be nice. Did you try something 
> > along this line?
> I haven't tried that exact formulation, but given the above issue with 
> `DenseMap`, I'm not sure it will work.  Happy to try once we get it ironed 
> out.
> 
> Another thing I've thought about is factoring the `functionDecl` matcher into 
> a separate function, because I expect it to be reused.  I haven't been able 
> to deduce what type that function would return.
the type is probably `clang::ast_matchers::internal::Matcher`. 
Not sure what the `bind` does with the type though.

If that is not the case you can make a nice error with `int Bad = 
functionDecl(anything());`, the type should be printed somewhere ;)
You could maybe create a lambda for that matcher or an `AST_MATCHER(...)` which 
some checks do as well. All variants are in use with clang-tidy.



Comment at: clang-tidy/abseil/DurationRewriter.h:20
+// Duration factory and conversion scales
+enum class DurationScale {
+  Hours,

You could specify the underlying type to `std::int8` directly, making the enum 
smaller and a more space-efficient `DenseMap`.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Sorry it's taken so long to get all the feedback addressed!




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

JonasToth wrote:
> I think this could be made a `DenseMap` with just the right amount of dense 
> entries. 12 elements seems not too much to me.
> Does the key-type need to be `std::string`, or could it be `StringRef`(or 
> `StringLiteral` making everything `constexpr` if possible)?
> Is there some strange stuff with dangling pointers or other issues going on?
Conceptually, this could easily be `constexpr`, but my compiler doesn't seem to 
want to build something which is called `static constexpr 
llvm::DenseMap`.  It's chief complaint is that 
such a type has a non-trivial destructor. Am I using this correctly?



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map>
+  InverseMap(

JonasToth wrote:
> This variable is a little hard to read. Could you make a little 
> wrapper-struct instead of the `tuple` to make clear which element represents 
> what?
> Otherwise, why not `std::pair`?
> 
> - same `DenseMap` argument
> - same `StringRef`/`StringLiteral` instead `string` consideration
`std::pair` works here.

I'll defer the `DenseMap` and `StringRef`/`StringLiteral` changes until we 
determine if they are actually possible.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:68
+
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.

JonasToth wrote:
> The basis for this "we know" might change in the future if `abs::Duration` 
> does things differently. Is adding stuff allowed in the `absl::` space? (i am 
> not fluent with the guarantees that it gives). You could maybe `assert` that 
> the find is always correct, depending on `absl` guarantees.
`absl::` could add things.  In this case, I'm very confident they won't, but 
I've still added the assert.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:108
+
+  // TODO(hwright): Check for another condition:
+  //  1. duration-factory-scale

JonasToth wrote:
> in LLVM the TODO does not contain a name for the author.
I just removed the TODO (this isn't a required part of the check).



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+  hasAnyName(
+  "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+  "::absl::ToDoubleSeconds", 
"::absl::ToDoubleMilliseconds",

JonasToth wrote:
> the list here is somewhat duplicated with the static members in the functions 
> at the top. it would be best to merge them.
> Not sure on how much `constexpr` is supported by the llvm-datastructures, but 
> a constexpr `DenseMap.keys()` would be nice. Did you try something along this 
> line?
I haven't tried that exact formulation, but given the above issue with 
`DenseMap`, I'm not sure it will work.  Happy to try once we get it ironed out.

Another thing I've thought about is factoring the `functionDecl` matcher into a 
separate function, because I expect it to be reused.  I haven't been able to 
deduce what type that function would return.


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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 175379.
hwright marked 23 inline comments as done.

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

https://reviews.llvm.org/D54737

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationComparisonCheck.h
  clang-tidy/abseil/DurationDivisionCheck.h
  clang-tidy/abseil/DurationFactoryFloatCheck.cpp
  clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-comparison.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-comparison.cpp

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %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 d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-comparison.rst:11
+compared against a floating-pointer value, truncation during the ``Duration``
+conversion might yield a different result.  In practice this is very rare, and
+still indicates a bug which should be fixed.

Please fix double space.



Comment at: docs/clang-tidy/checks/abseil-duration-comparison.rst:34
+  if (absl::Microseconds(x) < d) ...
+..

Please remove ..


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/abseil-duration-comparison.cpp:89
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS

JonasToth wrote:
> What would happen for a type, that can implicitly convert to a duration or 
> double/int.
> 
> ```
> struct MetaBenchmarkResults {
> int64_t RunID;
>absl::Duration D;
> 
>   operator absl::Duration() { return D; }
> };
> 
> auto R = MetaBenchmarkResults { /* Foo */ };
> bool WithinReason = Threshold < R;
> ```
> 
> What is the correct behaviour there? I think this should be diagnosed too.
H, I fiddled around in godbolt, and i think there is nothing to trick 
around with conversions, as they wont compile.
If you find something along the lines, please tell me. But i guess this is a 
non-issue.

https://godbolt.org/z/gqhhHo



Comment at: test/clang-tidy/abseil-duration-comparison.cpp:153
+  // Other abseil-duration checks folded into this one
+  b = static_cast(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in 
the duration domain [abseil-duration-comparison]

Please test the other casts you match on as well (especially c-style)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please upload the diff with full context? Especially the parts with 
refactoring are harder to judge if the code around is not visible. Thank you :)




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

I think this could be made a `DenseMap` with just the right amount of dense 
entries. 12 elements seems not too much to me.
Does the key-type need to be `std::string`, or could it be `StringRef`(or 
`StringLiteral` making everything `constexpr` if possible)?
Is there some strange stuff with dangling pointers or other issues going on?



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map>
+  InverseMap(

This variable is a little hard to read. Could you make a little wrapper-struct 
instead of the `tuple` to make clear which element represents what?
Otherwise, why not `std::pair`?

- same `DenseMap` argument
- same `StringRef`/`StringLiteral` instead `string` consideration



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:68
+
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.

The basis for this "we know" might change in the future if `abs::Duration` does 
things differently. Is adding stuff allowed in the `absl::` space? (i am not 
fluent with the guarantees that it gives). You could maybe `assert` that the 
find is always correct, depending on `absl` guarantees.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:78
+DurationScale Scale, const Expr ) {
+  auto InverseFunctions = getInverseForScale(Scale);
+  if (auto MaybeCallArg = selectFirst(

Please no `auto` here



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:79
+  auto InverseFunctions = getInverseForScale(Scale);
+  if (auto MaybeCallArg = selectFirst(
+  "e", match(callExpr(callee(functionDecl(hasAnyName(

Please use `const auto*` to make it clear that this is a pointer



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:99
+  // First check to see if we can undo a complimentary function call.
+  if (auto MaybeRewrite =
+  maybeRewriteInverseDurationCall(Result, Scale, RootNode)) {

please no `auto`, you can ellide the braces



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:104
+
+  if (IsLiteralZero(Result, RootNode)) {
+return "absl::ZeroDuration()";

ellide braces



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:108
+
+  // TODO(hwright): Check for another condition:
+  //  1. duration-factory-scale

in LLVM the TODO does not contain a name for the author.



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+  hasAnyName(
+  "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+  "::absl::ToDoubleSeconds", 
"::absl::ToDoubleMilliseconds",

the list here is somewhat duplicated with the static members in the functions 
at the top. it would be best to merge them.
Not sure on how much `constexpr` is supported by the llvm-datastructures, but a 
constexpr `DenseMap.keys()` would be nice. Did you try something along this 
line?



Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:151
+  // if nothing needs to be done.
+  auto lhs_replacement =
+  rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());

Please follow the naming convention and don't use `auto` here.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+  llvm::Optional SimpleArg = stripFloatCast(Result, *Arg);
+  if (!SimpleArg) {
+SimpleArg = stripFloatLiteralFraction(Result, *Arg);

braces can be ellided here



Comment at: clang-tidy/abseil/DurationRewriter.cpp:51
+bool IsLiteralZero(const MatchFinder::MatchResult , const Expr ) {
+  return selectFirst(
+  "val", match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),

This is a implicit pointer-to-bool-conversion, isn't it? I think for 
readability purposes this should be made clear with a binary comparison.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:61
+   const Expr ) {
+  if (auto MaybeCastArg = selectFirst(
+  "cast_arg",

`const auto *`



Comment at: clang-tidy/abseil/DurationRewriter.cpp:95
+  // Check for an explicit cast to `float` or `double`.
+  if (auto MaybeArg = stripFloatCast(Result, Node)) {
+return *MaybeArg;