[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<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const std::unordered_map<std::string, DurationScale> 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<DurationScale,
+                                  std::tuple<std::string, std::string>>
+      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 &Node) {
+  auto InverseFunctions = getInverseForScale(Scale);
+  if (auto MaybeCallArg = selectFirst<const Expr>(
----------------
Please no `auto` here


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:79
+  auto InverseFunctions = getInverseForScale(Scale);
+  if (auto MaybeCallArg = selectFirst<const Expr>(
+          "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<std::string> 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 &Result, const Expr &Node) {
+  return selectFirst<const clang::Expr>(
+      "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 &Node) {
+  if (auto MaybeCastArg = selectFirst<const Expr>(
+          "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;
----------------
please no `auto`, braces


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:100
+  // Check for floats without fractional components.
+  if (auto MaybeArg = stripFloatLiteralFraction(Result, Node)) {
+    return *MaybeArg;
----------------
same


================
Comment at: docs/clang-tidy/checks/abseil-duration-comparison.rst:10
+N.B.: In cases where a ``Duration`` was being converted to an integer and then
+compared against a floating-pointer value, truncation during the ``Duration``
+conversion might yield a different result.  In practice this is very rare, and
----------------
'floating-pointer' -> 'floating-point'?


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:68
+  // 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]
----------------
The test-cases miss macros and templates, please add reasonable cases for each of these categories.


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:89
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
----------------
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.


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:161
+  // These should not match
+  b = 6 < 4;
+}
----------------
could you please add a cases similiar to this:

```
if (some_condition && very_very_very_long_variable_name
     < SomeDuration) {
  // ...
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54737