Page MenuHomePhabricator

[clang-tidy] bugprone-assert-side-effect: Warn on NSAssert by default.
ClosedPublic

Authored by NoQ on Jan 27 2021, 5:41 AM.

Details

Summary

This is basically standard Objective-C assert. While we could always pass it as an option, i believe it makes perfect sense to warn on it by default.

The same applies to NSCAssert which drops additional support for displaying Objective-C method information so it acts exactly like C assert.

I tested it on live code with actual Foundation headers and it seems to work correctly out of the box.

I'm also interested in making an even more agressive step in this direction and adding an option to accept any macro that ends with "...assert" suffix, case-insensitively. This would probably go under an off-by-default flag. Would this be acceptable?

Diff Detail

Event Timeline

NoQ created this revision.Jan 27 2021, 5:41 AM

LGTM, but it would be great if code owners take a look as well.

aaron.ballman accepted this revision.Jan 27 2021, 5:55 AM
aaron.ballman added a subscriber: aaron.ballman.

LGTM!

I'm also interested in making an even more agressive step in this direction and adding an option to accept any macro that ends with "...assert" suffix, case-insensitively. This would probably go under an off-by-default flag. Would this be acceptable?

I think that makes sense to me. We should probably look at popular OS SDKs and CRTs to see what creative ways they spell assertion macros to make sure those are covered as well, like https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-asserte-assert-expr-macros?view=msvc-160 and https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-assertmsg

This revision is now accepted and ready to land.Jan 27 2021, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 10:32 PM