[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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?vs=170912=170932#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53339
Files:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-factory-float.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-factory-float.cpp
Index: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.h
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.h
@@ -0,0 +1,38 @@
+//===--- DurationFactoryFloatCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYFLOATCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYFLOATCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check finds cases where `Duration` factories are being called with
+/// floating point arguments, but could be called using integer arguments.
+/// It handles explicit casts and floating point literals with no fractional
+/// component.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-factory-float.html
+class DurationFactoryFloatCheck : public ClangTidyCheck {
+public:
+ DurationFactoryFloatCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYFLOATCHECK_H
Index: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
@@ -0,0 +1,106 @@
+//===--- DurationFactoryFloatCheck.cpp - clang-tidy ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DurationFactoryFloatCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
+static llvm::Optional
+truncateIfIntegral(const FloatingLiteral ) {
+ double Value = FloatLiteral.getValueAsApproximateDouble();
+ if (std::fmod(Value, 1) == 0) {
+if (Value >= static_cast(1u << 31))
+ return llvm::None;
+
+return llvm::APSInt::get(static_cast(Value));
+ }
+ return llvm::None;
+}
+
+// Returns `true` if `Range` is inside a macro definition.
+static bool InsideMacroDefinition(const MatchFinder::MatchResult ,
+ SourceRange Range) {
+ return !clang::Lexer::makeFileCharRange(
+ clang::CharSourceRange::getCharRange(Range),
+ *Result.SourceManager, Result.Context->getLangOpts())
+ .isValid();
+}
+
+void DurationFactoryFloatCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(
+ callee(functionDecl(hasAnyName(
+ "absl::Nanoseconds", "absl::Microseconds", "absl::Milliseconds",
+ "absl::Seconds", "absl::Minutes", "absl::Hours"))),
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
+hasSourceExpression(expr().bind("cast_arg"))),
+cStyleCastExpr(
+
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 somebody else will have to commit for me. :) https://reviews.llvm.org/D53339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds((double) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes((float) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds(double(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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) diagnostics don't end with a period. This one doesn't > need to be special in this regard. Same below. I believe that was related to a comment to make it a full sentence with grammar and so on, AFAIK these are supposed to be correct. The diagnostic message itself not, I did not intend to include it. https://reviews.llvm.org/D53339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 with a period. This one doesn't need to be special in this regard. Same below. https://reviews.llvm.org/D53339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 wrote:
> hokein wrote:
> > nit: clang-tidy message is not a complete sentence, use lower letter `use`.
> This is a complete sentence, in the imperative style of other clang-tidy
> checks. I've added a full-stop at the end.
Ah, sorry for not clarifying it clearly. In clang-tidy, we don't use complete
sentence for warnings, so just remove the "." at the end.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 duration-related check
> `abseil-duration-division`, for consistency.
>
> `clang-tidy/rename_check.py` may help you.
The expectation is that there may be other checks relating to calls to duration
factories, beyond just floating point literal and cast simplification.
Though I'm happy to be convinced otherwise.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:94
+ diag(MatchedCall->getBeginLoc(),
+ (llvm::Twine("Use integer version of absl::") +
+MatchedCall->getDirectCallee()->getName())
hokein wrote:
> nit: clang-tidy message is not a complete sentence, use lower letter `use`.
This is a complete sentence, in the imperative style of other clang-tidy
checks. I've added a full-stop at the end.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds((double) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes((float) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds(double(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 a duration-related check
`abseil-duration-division`, for consistency.
`clang-tidy/rename_check.py` may help you.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:94
+ diag(MatchedCall->getBeginLoc(),
+ (llvm::Twine("Use integer version of absl::") +
+MatchedCall->getDirectCallee()->getName())
nit: clang-tidy message is not a complete sentence, use lower letter `use`.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds((double) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes((float) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds(double(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 wrote:
> This function name doesn't seem to relate to the behavior of the function?
> Rather than try to pin it down like that, perhaps rename to
> "CanRangeBeSafelyReplaced()` and add a comment as to why this is the correct
> approach for answering the question.
I've added a comment describing what the function does, but leaving the name
the same.
Since this isn't the only reason why we reject a given range, and I think
keeping those decisions co-located is probably best, I'd like to leave that
logic in the `check` method below, rather than moving part of it up 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
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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(static_cast(Value));` instead
-- it should do the same thing.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:36
+
+static bool InsideMacroDefinition(const MatchFinder::MatchResult ,
+ SourceRange Range) {
This function name doesn't seem to relate to the behavior of the function?
Rather than try to pin it down like that, perhaps rename to
"CanRangeBeSafelyReplaced()` and add a comment as to why this is the correct
approach for answering the question.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:54
+hasSourceExpression(expr().bind("cast_arg"))),
+cStyleCastExpr(
+hasDestinationType(realFloatingPointType()),
What about function-style casts? e.g. `float(1)`
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:74
+
+ // Check for static casts to `float`.
+ if (const auto *MaybeCastArg = Result.Nodes.getNodeAs("cast_arg")) {
Comment is now out of date, this checks for a few kinds of casts.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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-tidy/abseil/DurationFactoryFloatCheck.cpp:51
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
JonasToth wrote:
> Nit: the duplication in the cast matcher can be removed with a variable `auto
> CastToFloat = hasDestinationType(realFloatingPointType()),
> hasSourceExpression(expr().bind("cast_arg"));` and then used in the matcher.
This turns out to be harder than it looks: `auto` can't be used here because it
would be ambiguous, and the actual type is part of the `internal` namespace so
I'm hesitant to use it here.
I've left the duplication as-is; if there's a better way to de-dupe, I'm happy
to try it.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:99
+}
+ }
+}
JonasToth wrote:
> is it logically valid to fall reach the end of the method?
> If not please add an `llvm_unreachable()`
It is. Any case we don't catch would fall through to the end of this function.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 or so. Do you have commit access?
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:51
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
Nit: the duplication in the cast matcher can be removed with a variable `auto
CastToFloat = hasDestinationType(realFloatingPointType()),
hasSourceExpression(expr().bind("cast_arg"));` and then used in the matcher.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:99
+}
+ }
+}
is it logically valid to fall reach the end of the method?
If not please add an `llvm_unreachable()`
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 into ...` or similar. I've added some more text. I'd prefer not to make this duplicate much of what is already in the user-facing documentation, since that should be more complete and canonical (and is already included by reference). https://reviews.llvm.org/D53339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,124 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ d = absl::Seconds((double) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes((float) 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ // This should not be flagged
+ d = absl::Seconds(static_cast(5.0));
+ d = absl::Seconds((int) 5.0);
+}
Index: docs/clang-tidy/checks/list.rst
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
JonasToth added inline comments.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:51
+ hasArgument(0,
+ anyOf(cxxStaticCastExpr(
+hasDestinationType(realFloatingPointType()),
What about c-style casts?
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:67
+ const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts();
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
Please clarfiy this comment a bit more, like `Marcros as arguments are
ignored.` or the like.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:86
+ Result.Nodes.getNodeAs("float_literal")) {
+// Attempt to simplify a Duration factory call with a literal argument.
+if (llvm::Optional IntValue = truncateIfIntegral(*LitFloat))
{
please highlight the code construct ('Duration' in this case) here and in other
comments to clarify its about the class in user-code.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.h:19
+
+/// Prefer integer Duration factories when possible.
+///
Please add more to the doc here, like `This check finds ... and transforms
these calls into ...` or similar.
Comment at: docs/clang-tidy/checks/abseil-duration-factory-float.rst:6
+
+Finds cases where callers of ``absl::Duration`` factory functions (such as
+``absl::Seconds`` or ``absl::Hours``) are providing a floating point value
Please synchronize the first paragraph with the release notes (I would prefer
the release notes version)
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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. https://reviews.llvm.org/D53339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 suggests that macros are
> > already filtered out?
> This is a different check than above.
>
> In the first case, we want to be sure we aren't replacing cases inside of a
> macro, such as:
> ```
> #define SECONDS(x) absl::Seconds(x)
> SECONDS(1.0)
> ```
>
> In this one, we want to avoid changing the argument if it is itself a macro:
> ```
> #define VAL 1.0
> absl::Seconds(VAL);
> ```
>
> So it is a separate check, not just a re-assertion of the first one.
Ok, I misunderstood the code then :)
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+ absl::Duration d;
+
hwright wrote:
> JonasToth wrote:
> > Could you also provide test cases with hexadecimal floating literals, which
> > are C++17? The thousand-separators could be checked as well (dunno the
> > correct term right now, but the `1'000'000` feature).
> > Please add test cases for negative literals, too.
> Added the hexadecimal floating literal tests.
>
> The testing infrastructure wouldn't build the test source with `3'000` as a
> literal argument. (There's also an argument that by the time we get to the
> AST, that distinction is gone anyway and this test isn't the place to check
> comprehensive literal parsing.)
>
> I've also added a negative literal test, to illustrate that we don't yet
> handle that case (which is in practice pretty rare). I'd rather add it in a
> separate change.
I am happy with that. I agree that parsing should deal with it fine and your
code seems to do it fine as well. My experience is, that sometimes there are
still surprises :)
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 use `std::uint64_t` instead to correspond to
> the line above? And where does the signedness difference come from?
> (`/*isUnsigned=*/false`)
I don't remember where the signedness difference came from, so I've made this
`std::int64_t`.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+return;
JonasToth wrote:
> maybe `assert` instead, as your comment above suggests that macros are
> already filtered out?
This is a different check than above.
In the first case, we want to be sure we aren't replacing cases inside of a
macro, such as:
```
#define SECONDS(x) absl::Seconds(x)
SECONDS(1.0)
```
In this one, we want to avoid changing the argument if it is itself a macro:
```
#define VAL 1.0
absl::Seconds(VAL);
```
So it is a separate check, not just a re-assertion of the first one.
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+ absl::Duration d;
+
JonasToth wrote:
> Could you also provide test cases with hexadecimal floating literals, which
> are C++17? The thousand-separators could be checked as well (dunno the
> correct term right now, but the `1'000'000` feature).
> Please add test cases for negative literals, too.
Added the hexadecimal floating literal tests.
The testing infrastructure wouldn't build the test source with `3'000` as a
literal argument. (There's also an argument that by the time we get to the
AST, that distinction is gone anyway and this test isn't the place to check
comprehensive literal parsing.)
I've also added a negative literal test, to illustrate that we don't yet handle
that case (which is in practice pretty rare). I'd rather add it in a separate
change.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,115 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(0x3.p1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(6);
+
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+ d = absl::Seconds(0x3.4p1);
+
+ // Negative literals (we don't yet handle this case)
+ d = absl::Seconds(-3.0);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertStaticCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ // This should not be flagged
+ d = absl::Seconds(static_cast(5.0));
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,6 +5,7 @@
.. toctree::
abseil-duration-division
+ abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
Index: docs/clang-tidy/checks/abseil-duration-factory-float.rst
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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, global, function parameter) use exactly same naming
convention `CamelCase`.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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_t` instead to correspond to the
line above? And where does the signedness difference come from?
(`/*isUnsigned=*/false`)
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+ // Don't try and replace things inside of macro definitions.
+ if (!clang::Lexer::makeFileCharRange(
+ clang::CharSourceRange::getCharRange(MatchedCall->getSourceRange()),
That is worth a helper function taking a `SourceRange` as argument.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+return;
maybe `assert` instead, as your comment above suggests that macros are already
filtered out?
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:72
+
+ // Check for static casts to float
+ if (const auto *MaybeCastArg = Result.Nodes.getNodeAs("cast_arg")) {
missing full stop.
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:84
+
+ // Check for floats without fractional components
+ if (const auto *LitFloat =
missing full stop
Comment at: docs/clang-tidy/checks/list.rst:12
abseil-redundant-strcat-calls
- abseil-string-find-startswith
abseil-str-cat-append
spurious change
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+ absl::Duration d;
+
Could you also provide test cases with hexadecimal floating literals, which are
C++17? The thousand-separators could be checked as well (dunno the correct term
right now, but the `1'000'000` feature).
Please add test cases for negative literals, too.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 much, but would `std::modf` be more appropriate in
> this context?
I'm not actually sure. Since we're checking the remainder against `0`, we
don't need to also separately get the integral part, since if the conditional
passes, we know the original value //is// the integral part. It would seem
that `std::modf` just adds more complexity.
https://reviews.llvm.org/D53339
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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/DurationFactoryFloatCheck.cpp
clang-tidy/abseil/DurationFactoryFloatCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-float.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-float.cpp
Index: test/clang-tidy/abseil-duration-factory-float.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-float.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-float %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ConvertFloatTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(60.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(60);
+ d = absl::Minutes(300.0);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(300);
+
+ d = absl::Milliseconds(1e2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Milliseconds(100);
+ d = absl::Seconds(3.0f);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+ d = absl::Seconds(3.);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(3);
+
+ // Ignored expressions
+ d = absl::Seconds(.001);
+ d = absl::Seconds(.100);
+ d = ::absl::Seconds(1);
+ d = ::absl::Minutes(1);
+ d = ::absl::Hours(1);
+
+ // This is bigger than we can safely fit in a positive int32, so we ignore it.
+ d = absl::Seconds(1e12);
+
+ int x;
+ d = absl::Seconds(x);
+ float y;
+ d = absl::Minutes(y);
+
+#define SECONDS(x) absl::Seconds(x)
+ SECONDS(60);
+#undef SECONDS
+#define THIRTY 30.0
+ d = absl::Seconds(THIRTY);
+#undef THIRTY
+}
+
+template
+void InTemplate() {
+ absl::Duration d;
+
+ d = absl::Seconds(N); // 1
+ // ^ No replacement here.
+
+ d = absl::Minutes(1.0); // 2
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(1); // 2
+}
+
+void Instantiate() {
+ InTemplate<60>();
+ InTemplate<1>();
+}
+
+void ConvertStaticCastTest() {
+ absl::Duration d;
+
+ d = absl::Seconds(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Seconds(5);
+
+ d = absl::Minutes(static_cast(5));
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float]
+ // CHECK-FIXES: absl::Minutes(5);
+
+ // This should not be flagged
+ d = absl::Seconds(static_cast(5.0));
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,12 +5,13 @@
.. toctree::
abseil-duration-division
+ abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
- abseil-string-find-startswith
abseil-str-cat-append
+ abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-duration-factory-float.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-duration-factory-float.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - abseil-duration-factory-float
+
+abseil-duration-factory-float
+=
+
+Finds cases where callers of
[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check
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 mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
