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:
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
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
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)
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
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::") +
+
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
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
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
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
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 ,
+ 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
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:
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
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
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
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:51
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
What about
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
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
zturner added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:24
+truncateIfIntegral(const FloatingLiteral ) {
+ double value = FloatLiteral.getValueAsApproximateDouble();
+ if (std::fmod(value, 1) == 0) {
All variables (local,
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
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
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
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
30 matches
Mail list logo
