[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Alright. Thank you for clarification :)



================
Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:75
+  const auto &Assign = *Result.Nodes.getNodeAs<BinaryOperator>("assign");
+  const auto &RHS = *cast<BinaryOperator>(Assign.getRHS()->IgnoreParenCasts());
+  assert(RHS.isComparisonOp());
----------------
You can match `RHS` in your matcher by adding another `bind` for the binaryOperator.


================
Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:76
+  const auto &RHS = *cast<BinaryOperator>(Assign.getRHS()->IgnoreParenCasts());
+  assert(RHS.isComparisonOp());
+
----------------
this assertion is redundant with the new matcher.


================
Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:78
+
+  diag(RHS.getOperatorLoc(),
+       "Top-level comparisons should be moved out of TEMP_FAILURE_RETRY");
----------------
You could even provide a fixit to do this. But this can be done in later patches, too.


================
Comment at: test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c:1
+// RUN: %check_clang_tidy %s bugprone-comparison-in-temp-failure-retry %t
+
----------------
Could you please add a test with control structures, like loops you used in your example?


================
Comment at: test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c:5
+  ({                                                                           \
+    typeof(x) __z;                                                             \
+    do                                                                         \
----------------
I think you could add one test, that shows using `long int` instead of `typeof` is diagnosed, given glibc uses this approach (or even copy there macro)


https://reviews.llvm.org/D45059