This revision was automatically updated to reflect the committed changes.
Closed by commit rL329759: [clang-tidy] Add a
`android-comparison-in-temp-failure-retry` check (authored by gbiv, committed
by ).
Herald added subscribers: llvm-commits, klimek.
Changed prior to commit:
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG. Thanks!
https://reviews.llvm.org/D45059
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv updated this revision to Diff 141768.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Address feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
george.burgess.iv added a comment.
Thanks!
I plan to commit this tomorrow, to give time for any last-minute comments.
Thanks again to everyone for the review. :)
https://reviews.llvm.org/D45059
___
cfe-commits mailing list
aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM with a few minor nits to be fixed.
Comment at:
docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst:4
+android-comparison-in-temp-failure-retry
+=
george.burgess.iv updated this revision to Diff 141687.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Herald added a subscriber: srhines.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/android/AndroidTidyModule.cpp
george.burgess.iv added a comment.
Thanks for the feedback!
> and I suspect that your interest in this check is also related to android?
Yup :)
> Alternatively, if there are other checks specific to the GNU C library
> planned, we could add a new module for it.
I have nothing planned, so I'm
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
The TEMP_FAILURE_RETRY macro is specific to the GNU C library (and environments
that attempt to mimic it). The generic bugprone- module is not the best place
for this check. I
JonasToth added a comment.
LGTM, but @aaron.ballman, @alexfh or someone else should review it before
comitting.
https://reviews.llvm.org/D45059
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv added inline comments.
Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:78
+
+ diag(RHS.getOperatorLoc(),
+ "Top-level comparisons should be moved out of TEMP_FAILURE_RETRY");
JonasToth wrote:
> You could even
george.burgess.iv updated this revision to Diff 140675.
george.burgess.iv marked 5 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
JonasToth added a comment.
Alright. Thank you for clarification :)
Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:75
+ const auto = *Result.Nodes.getNodeAs("assign");
+ const auto = *cast(Assign.getRHS()->IgnoreParenCasts());
+
george.burgess.iv added a comment.
> C89 has no bool type and no stdbool.h but it has been added to later
> standards? That means the generalization could theoretically be done for
> later C standards, because we could check if the bool typedef had been used?
> If yes, would the check benefit?
JonasToth added a comment.
> I'm not quite sure how we would go about that with confidence. The code we'd
> need to catch looks something like:
>
> typeof(foo() == y) x;
> /* snip */
> bar(x == -1); // warning: comparison is pointless
>
> If we could tell that x's type was inferred from a
george.burgess.iv added a comment.
Thanks for the feedback!
> You noted, that the C++ warning would catch this case, but does not get
> triggered in C. Would it be wort to generalize the concern and have a warning
> that catch the comparison, regardless of the macro?
I'm not quite sure how we
george.burgess.iv updated this revision to Diff 140459.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
JonasToth added a comment.
You noted, that the C++ warning would catch this case, but does not get
triggered in C. Would it be wort to generalize the concern and have a warning
that catch the comparison, regardless of the macro?
Do other major libraries define a similar macro but name it
george.burgess.iv added a comment.
Thanks!
> Will be good idea to clarify where TEMP_FAILURE_RETRY come from.
Updated the CL summary and added "TEMP_FAILURE_RETRY is a macro provided by
both glibc and Bionic." to ComparisonInTempFailureRetryCheck.h. Did you have
anything else in mind?
george.burgess.iv updated this revision to Diff 140324.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv edited the summary of this revision.
george.burgess.iv added a comment.
Addressed feedback.
https://reviews.llvm.org/D45059
Files:
Eugene.Zelenko added a comment.
Will be good idea to clarify where TEMP_FAILURE_RETRY come from.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45059
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
20 matches
Mail list logo
