[clang-tools-extra] r349636 - [clang-tidy] Diagnose abseil-duration-comparison on macro arguments
Hyrum Wright via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 19 08:03:35 PST 2018
Author: hwright
Date: Wed Dec 19 08:03:34 2018
New Revision: 349636
URL: http://llvm.org/viewvc/llvm-project?rev=349636&view=rev
Log:
[clang-tidy] Diagnose abseil-duration-comparison on macro arguments
Summary:
This change relaxes the requirements on the utility
`rewriteExprFromNumberToDuration` function, and introduces new checking
inside of the `abseil-duration-comparison` check to allow macro argument
expression transformation.
Differential Revision: https://reviews.llvm.org/D55784
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=349636&r1=349635&r2=349636&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Wed Dec 19 08:03:34 2018
@@ -19,6 +19,26 @@ namespace clang {
namespace tidy {
namespace abseil {
+/// Return `true` if `E` is a either: not a macro at all; or an argument to
+/// one. In the latter case, we should still transform it.
+static bool IsValidMacro(const MatchFinder::MatchResult &Result,
+ const Expr *E) {
+ if (!E->getBeginLoc().isMacroID())
+ return true;
+
+ SourceLocation Loc = E->getBeginLoc();
+ // We want to get closer towards the initial macro typed into the source only
+ // if the location is being expanded as a macro argument.
+ while (Result.SourceManager->isMacroArgExpansion(Loc)) {
+ // We are calling getImmediateMacroCallerLoc, but note it is essentially
+ // equivalent to calling getImmediateSpellingLoc in this context according
+ // to Clang implementation. We are not calling getImmediateSpellingLoc
+ // because Clang comment says it "should not generally be used by clients."
+ Loc = Result.SourceManager->getImmediateMacroCallerLoc(Loc);
+ }
+ return !Loc.isMacroID();
+}
+
void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
auto Matcher =
binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
@@ -35,10 +55,6 @@ void DurationComparisonCheck::registerMa
void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
- // Don't try to replace things inside of macro definitions.
- if (Binop->getExprLoc().isMacroID())
- return;
-
llvm::Optional<DurationScale> Scale = getScaleForInverse(
Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
if (!Scale)
@@ -48,19 +64,19 @@ void DurationComparisonCheck::check(cons
// want to handle the case of rewriting both sides. This is much simpler if
// we unconditionally try and rewrite both, and let the rewriter determine
// if nothing needs to be done.
- llvm::Optional<std::string> LhsReplacement =
+ if (!IsValidMacro(Result, Binop->getLHS()) ||
+ !IsValidMacro(Result, Binop->getRHS()))
+ return;
+ std::string LhsReplacement =
rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
- llvm::Optional<std::string> RhsReplacement =
+ std::string RhsReplacement =
rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
- if (!(LhsReplacement && RhsReplacement))
- return;
-
diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
<< FixItHint::CreateReplacement(Binop->getSourceRange(),
- (llvm::Twine(*LhsReplacement) + " " +
+ (llvm::Twine(LhsReplacement) + " " +
Binop->getOpcodeStr() + " " +
- *RhsReplacement)
+ RhsReplacement)
.str());
}
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp?rev=349636&r1=349635&r2=349636&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp Wed Dec 19 08:03:34 2018
@@ -183,14 +183,11 @@ llvm::Optional<DurationScale> getScaleFo
return ScaleIter->second;
}
-llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+std::string rewriteExprFromNumberToDuration(
const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
const Expr *Node) {
const Expr &RootNode = *Node->IgnoreParenImpCasts();
- if (RootNode.getBeginLoc().isMacroID())
- return llvm::None;
-
// First check to see if we can undo a complimentary function call.
if (llvm::Optional<std::string> MaybeRewrite =
rewriteInverseDurationCall(Result, Scale, RootNode))
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h?rev=349636&r1=349635&r2=349636&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Wed Dec 19 08:03:34 2018
@@ -65,7 +65,7 @@ llvm::Optional<DurationScale> getScaleFo
/// Assuming `Node` has type `double` or `int` representing a time interval of
/// `Scale`, return the expression to make it a suitable `Duration`.
-llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+std::string rewriteExprFromNumberToDuration(
const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
const Expr *Node);
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp?rev=349636&r1=349635&r2=349636&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp Wed Dec 19 08:03:34 2018
@@ -42,10 +42,8 @@ void DurationSubtractionCheck::check(con
if (!Scale)
return;
- llvm::Optional<std::string> RhsReplacement =
+ std::string RhsReplacement =
rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
- if (!RhsReplacement)
- return;
const Expr *LhsArg = Result.Nodes.getNodeAs<Expr>("lhs_arg");
@@ -54,7 +52,7 @@ void DurationSubtractionCheck::check(con
Binop->getSourceRange(),
(llvm::Twine("absl::") + FuncDecl->getName() + "(" +
tooling::fixit::getText(*LhsArg, *Result.Context) + " - " +
- *RhsReplacement + ")")
+ RhsReplacement + ")")
.str());
}
Modified: clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp?rev=349636&r1=349635&r2=349636&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp Wed Dec 19 08:03:34 2018
@@ -127,6 +127,33 @@ void f() {
// CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
// CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
+ // We should still transform the expression inside this macro invocation
+#define VALUE_IF(v, e) v ? (e) : 0
+ int a = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1));
+ // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1);
+#undef VALUE_IF
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
+ int a2 = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1));
+ // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the duration domain [abseil-duration-comparison]
+ // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
+ int a3 = VALUE_IF(1, d1, Double);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
+ int a4 = VALUE_IF(1, d1, Double);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
// These should not match
b = 6 < 4;
More information about the cfe-commits
mailing list