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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
