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

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/

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

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. >

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

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

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

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:

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

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] D54737: [clang-tidy] Add the abseil-duration-comparison check

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

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

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

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

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

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

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:

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

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

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

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

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

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: > > >

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

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

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

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 {

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

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: >

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

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: >

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

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

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

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

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

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

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

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",

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

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},

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

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

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

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,

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

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

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

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},

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

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},

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

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},

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

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},

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

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

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

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

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

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

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

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

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

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( +

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

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

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

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

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

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.

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

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