[clang-tools-extra] 9d40fb8 - Allow to specify macro names for android-comparison-in-temp-failure-retry
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 10:12:29 PDT 2020
Author: Florian Mayer
Date: 2020-10-01T10:09:26-07:00
New Revision: 9d40fb808fd0fbd33eb3b50c20d7f402de5db91e
URL: https://github.com/llvm/llvm-project/commit/9d40fb808fd0fbd33eb3b50c20d7f402de5db91e
DIFF: https://github.com/llvm/llvm-project/commit/9d40fb808fd0fbd33eb3b50c20d7f402de5db91e.diff
LOG: Allow to specify macro names for android-comparison-in-temp-failure-retry
Some projects do not use the TEMP_FAILURE_RETRY macro but define their
own one, as not to depend on glibc / Bionic details. By allowing the
user to override the list of macros, these projects can also benefit
from this check.
Differential Revision: https://reviews.llvm.org/D83144
Added:
clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
Modified:
clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
index 188d44da51d81..c7b9896c64f81 100644
--- a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
+++ b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
@@ -18,32 +18,17 @@ namespace clang {
namespace tidy {
namespace android {
-namespace {
-AST_MATCHER(BinaryOperator, isRHSATempFailureRetryArg) {
- if (!Node.getBeginLoc().isMacroID())
- return false;
-
- const SourceManager &SM = Finder->getASTContext().getSourceManager();
- if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getBeginLoc()))
- return false;
-
- const LangOptions &Opts = Finder->getASTContext().getLangOpts();
- SourceLocation LocStart = Node.getBeginLoc();
- while (LocStart.isMacroID()) {
- SourceLocation Invocation = SM.getImmediateMacroCallerLoc(LocStart);
- Token Tok;
- if (!Lexer::getRawToken(SM.getSpellingLoc(Invocation), Tok, SM, Opts,
- /*IgnoreWhiteSpace=*/true)) {
- if (Tok.getKind() == tok::raw_identifier &&
- Tok.getRawIdentifier() == "TEMP_FAILURE_RETRY")
- return true;
- }
+ComparisonInTempFailureRetryCheck::ComparisonInTempFailureRetryCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ RawRetryList(Options.get("RetryMacros", "TEMP_FAILURE_RETRY")) {
+ StringRef(RawRetryList).split(RetryMacros, ",", -1, false);
+}
- LocStart = Invocation;
- }
- return false;
+void ComparisonInTempFailureRetryCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "RetryMacros", RawRetryList);
}
-} // namespace
void ComparisonInTempFailureRetryCheck::registerMatchers(MatchFinder *Finder) {
// Both glibc's and Bionic's TEMP_FAILURE_RETRY macros structurally look like:
@@ -63,15 +48,43 @@ void ComparisonInTempFailureRetryCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
binaryOperator(hasOperatorName("="),
hasRHS(ignoringParenCasts(
- binaryOperator(isComparisonOperator()).bind("binop"))),
- isRHSATempFailureRetryArg()),
+ binaryOperator(isComparisonOperator()).bind("inner"))))
+ .bind("outer"),
this);
}
void ComparisonInTempFailureRetryCheck::check(
const MatchFinder::MatchResult &Result) {
- const auto &BinOp = *Result.Nodes.getNodeAs<BinaryOperator>("binop");
- diag(BinOp.getOperatorLoc(), "top-level comparison in TEMP_FAILURE_RETRY");
+ StringRef RetryMacroName;
+ const auto &Node = *Result.Nodes.getNodeAs<BinaryOperator>("outer");
+ if (!Node.getBeginLoc().isMacroID())
+ return;
+
+ const SourceManager &SM = *Result.SourceManager;
+ if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getBeginLoc()))
+ return;
+
+ const LangOptions &Opts = Result.Context->getLangOpts();
+ SourceLocation LocStart = Node.getBeginLoc();
+ while (LocStart.isMacroID()) {
+ SourceLocation Invocation = SM.getImmediateMacroCallerLoc(LocStart);
+ Token Tok;
+ if (!Lexer::getRawToken(SM.getSpellingLoc(Invocation), Tok, SM, Opts,
+ /*IgnoreWhiteSpace=*/true)) {
+ if (Tok.getKind() == tok::raw_identifier &&
+ llvm::is_contained(RetryMacros, Tok.getRawIdentifier())) {
+ RetryMacroName = Tok.getRawIdentifier();
+ break;
+ }
+ }
+
+ LocStart = Invocation;
+ }
+ if (RetryMacroName.empty())
+ return;
+
+ const auto &Inner = *Result.Nodes.getNodeAs<BinaryOperator>("inner");
+ diag(Inner.getOperatorLoc(), "top-level comparison in %0") << RetryMacroName;
// FIXME: FixIts would be nice, but potentially nontrivial when nested macros
// happen, e.g. `TEMP_FAILURE_RETRY(IS_ZERO(foo()))`
diff --git a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
index d12c999720707..7b000ab2f54f6 100644
--- a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
+++ b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
@@ -10,6 +10,9 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H
#include "../ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
namespace clang {
namespace tidy {
@@ -22,10 +25,14 @@ namespace android {
/// TEMP_FAILURE_RETRY is a macro provided by both glibc and Bionic.
class ComparisonInTempFailureRetryCheck : public ClangTidyCheck {
public:
- ComparisonInTempFailureRetryCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ ComparisonInTempFailureRetryCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ const std::string RawRetryList;
+ SmallVector<StringRef, 5> RetryMacros;
};
} // namespace android
diff --git a/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst b/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
index e4de4b04351d7..93112ee2bea64 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
@@ -34,3 +34,10 @@ If you encounter this, the fix is simple: lift the comparison out of the
while (TEMP_FAILURE_RETRY(read(STDIN_FILENO, cs, sizeof(cs))) != 0) {
// Do something with cs.
}
+
+Options
+-------
+
+.. option:: RetryMacros
+
+ A comma-separated list of the names of retry macros to be checked.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c b/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
new file mode 100644
index 0000000000000..dde03ddabbcb0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t -- -config="{CheckOptions: [{key: android-comparison-in-temp-failure-retry.RetryMacros, value: 'MY_TEMP_FAILURE_RETRY,MY_OTHER_TEMP_FAILURE_RETRY'}]}"
+
+#define MY_TEMP_FAILURE_RETRY(x) \
+ ({ \
+ typeof(x) __z; \
+ do \
+ __z = (x); \
+ while (__z == -1); \
+ __z; \
+ })
+
+#define MY_OTHER_TEMP_FAILURE_RETRY(x) \
+ ({ \
+ typeof(x) __z; \
+ do \
+ __z = (x); \
+ while (__z == -1); \
+ __z; \
+ })
+
+int foo();
+int bar(int a);
+
+void with_custom_macro() {
+ MY_TEMP_FAILURE_RETRY(foo());
+ MY_TEMP_FAILURE_RETRY(foo() == 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+ MY_TEMP_FAILURE_RETRY((foo()));
+ MY_TEMP_FAILURE_RETRY((int)(foo() == 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+ MY_TEMP_FAILURE_RETRY((bar(foo() == 1)));
+ MY_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:49: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+}
+
+void with_other_custom_macro() {
+ MY_OTHER_TEMP_FAILURE_RETRY(foo());
+ MY_OTHER_TEMP_FAILURE_RETRY(foo() == 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+ MY_OTHER_TEMP_FAILURE_RETRY((foo()));
+ MY_OTHER_TEMP_FAILURE_RETRY((int)(foo() == 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+ MY_OTHER_TEMP_FAILURE_RETRY((bar(foo() == 1)));
+ MY_OTHER_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:55: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+}
More information about the cfe-commits
mailing list