Page MenuHomePhabricator

Allow to specify macro names for android-comparison-in-temp-failure-retry.
ClosedPublic

Authored by fmayer on Jul 3 2020, 12:33 PM.

Details

Summary

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.

Diff Detail

Event Timeline

fmayer created this revision.Jul 3 2020, 12:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 12:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Concept and implementation LGTM. Please wait for comment from +alexfh before landing, since I think they have more ownership over clang-tidy in general than I do :)

Hey, any updates on this? :)

srhines added inline comments.
clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c
1 ↗(On Diff #275450)

Ideally there should either be another RUN macro here without your new options, or there should be a separate test/file for checking here, since we'd like to make sure that the default behavior still works.

28 ↗(On Diff #275450)

Don't you want a check for MY_TEMP_FAILURE_RETRY for cases like this too (to make sure that it is detecting them)?

fmayer updated this revision to Diff 291297.Sep 11 2020, 11:40 AM

Added new file for the tests. This way, we keep the tests for the normal case as well.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c
1 ↗(On Diff #275450)

Left this file alone, and added a new file for custom macros.

28 ↗(On Diff #275450)

Checking for more cases in the other file.

srhines accepted this revision.Sep 11 2020, 11:43 AM

Thanks for creating the new test, and for making this more flexible. Everything else looks good here.

This revision is now accepted and ready to land.Sep 11 2020, 11:43 AM
fmayer added a comment.EditedSep 11 2020, 12:24 PM

Thanks for the review! Should I wait for Alex to take a look, or is it fine like this?

Thanks for the review! Should I wait for Alex to take a look, or is it fine like this?

George added @alexfh a while ago, but if it's ok with you, maybe give him until Tuesday or Wednesday next week in case he wants to respond?

alexfh accepted this revision.Sep 14 2020, 1:15 AM

Looks good modulo comment.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
26

Are the extra parentheses required for the macro to work? Can it be just MY_TEMP_FAILURE_RETRY(foo()); ?

fmayer updated this revision to Diff 291520.Sep 14 2020, 2:29 AM

Added more simple cases to the test.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
26

Added the more simple case as well.

I am not very experienced with Phabricator, but I think this needs another approval for the latest revision :) Thanks in advance!

fmayer added a comment.Oct 1 2020, 5:35 AM

I do not have commit access. Could someone submit this patch please?