alexfh added a comment.
In https://reviews.llvm.org/D53339#1274561, @hwright wrote:
> @JonasToth I don't actually have commit privileges, so somebody else will
> have to commit for me. :)
You should definitely ask commit access.
Repository:
rL LLVM
https://reviews.llvm.org/D53339
JonasToth added a comment.
Thanks for the patch!
Repository:
rL LLVM
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345167: [clang-tidy] Add the abseil-duration-factory-float
check (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53339
hwright added a comment.
In https://reviews.llvm.org/D53339#1274478, @JonasToth wrote:
> I think accepted now? :)
> If you want I can commit for you and monitor the buildbot, if there are
> bigger problems I would come back to you.
@JonasToth I don't actually have commit privileges, so somebo
JonasToth added a comment.
I think accepted now? :)
If you want I can commit for you and monitor the buildbot, if there are bigger
problems I would come back to you.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llv
hwright updated this revision to Diff 170912.
hwright marked 5 inline comments as done.
hwright added a comment.
Remove full-stop
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:84
+
+ // Check for floats without fractional components
+ if (const auto *LitFloat =
alexfh wrote:
> JonasToth wrote:
> > missing full stop
> Clang-tidy (and clang) di
alexfh added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:84
+
+ // Check for floats without fractional components
+ if (const auto *LitFloat =
JonasToth wrote:
> missing full stop
Clang-tidy (and clang) diagnostics don't end wit
hokein accepted this revision.
hokein added a comment.
LGTM.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:94
+ diag(MatchedCall->getBeginLoc(),
+ (llvm::Twine("Use integer version of absl::") +
+MatchedCall->getDirectCallee()->getName()
hwright added inline comments.
Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:32
+CheckFactories.registerCheck(
+"abseil-duration-factory-float");
CheckFactories.registerCheck(
hokein wrote:
> Maybe drop the `factory`? we already have a durat
hwright updated this revision to Diff 170847.
hwright marked 2 inline comments as done.
hwright added a comment.
Update diagnostic text
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloatChec
hokein added a comment.
looks good, just a few nits.
Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:32
+CheckFactories.registerCheck(
+"abseil-duration-factory-float");
CheckFactories.registerCheck(
Maybe drop the `factory`? we already have
hwright updated this revision to Diff 170803.
hwright marked an inline comment as done.
hwright added a comment.
Address reviewer comments.
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloat
hwright marked 4 inline comments as done.
hwright added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:36
+
+static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
+ SourceRange Range) {
---
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:29-31
+llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+ApInt = static_cast(Value);
+return ApInt;
I believe you can do `return APSInt::get(sta
hwright marked 2 inline comments as done.
hwright added a comment.
I do not have commit privileges, so somebody else would need to submit it.
We've had a version of this check running over our internal codebase for
several months with no unexpected problems.
Comment at: clang
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
@alexfh you did comment before, do you want to add more? I have no issues left.
Please give alex the opportunity to react, but if he doesn't (he has a lot to
do) you can commit in 3 days
hwright added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.h:19
+
+/// Prefer integer Duration factories when possible.
+///
JonasToth wrote:
> Please add more to the doc here, like `This check finds ... and transforms
> these calls i
hwright updated this revision to Diff 170659.
hwright marked 5 inline comments as done.
hwright added a comment.
Address reviewer comments
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloatC
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:51
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
What about c-st
JonasToth added a comment.
In https://reviews.llvm.org/D53339#1269998, @hwright wrote:
> Ping.
>
> What are the next steps here?
Please mark all comments you consider resolved as 'Done', i think alex already
kinda accepted it, but given there were more comments he should take another
look.
hwright added a comment.
Ping.
What are the next steps here?
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+return;
hwright wrote:
> JonasToth wrote:
> > maybe `assert` instead, as your comment above sugge
hwright added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:34
+llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+ApInt = static_cast(value);
+if (is_negative)
JonasToth wrote:
> Wouldn't it make more sense to
hwright updated this revision to Diff 170083.
hwright marked 6 inline comments as done.
hwright added a comment.
Addressed reviewer comments.
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFlo
zturner added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:24
+truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
+ double value = FloatLiteral.getValueAsApproximateDouble();
+ if (std::fmod(value, 1) == 0) {
All variables
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:34
+llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+ApInt = static_cast(value);
+if (is_negative)
Wouldn't it make more sense to use `std::uint64_
hwright added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:27
+ double value = FloatLiteral.getValueAsApproximateDouble();
+ if (std::fmod(value, 1) == 0) {
+bool is_negative = false;
alexfh wrote:
> Probably doesn't matter m
hwright updated this revision to Diff 169946.
hwright marked 8 inline comments as done.
hwright added a comment.
Addressed review comments
https://reviews.llvm.org/D53339
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryFloatC
Eugene.Zelenko added a comment.
By the word, why this check could not be generalized to any function/method
which have floating-point and integer variants?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53339
___
cfe-commits mailin
30 matches
Mail list logo
