Rightpoint / RZAssert

Useful assertion macros from the fine folks at Raizlabs.
Other
7 stars 0 forks source link

Logging #16

Closed ZevEisenberg closed 9 years ago

ZevEisenberg commented 9 years ago

You may want to view this by commit, rather than in total, because of CocoaPods diffs. Alternatively, just merge #17 first, since it takes care of all the messy project changes.

Fixes #4 Fixes #9 Fixes #13 Fixes #14

ZevEisenberg commented 9 years ago

@mgorbach this is nearing readiness. Question: have a look at RZAssert.h. It's getting pretty messy, with a lot of repetition, and it's pretty unreadable. It also has one method (+logMessageWithFormat:) that shouldn't be public, and is only here because the macros need to use it. Since the RZASSERT_* macros are now supposed to run all the time, not just in debug builds, what would you say to the idea of changing them all to functions, and hiding all the macro ugliness in the .m file?

mgorbach commented 9 years ago

You should really try to clean up the repetition here since it's asking for bugs and pain in the future. Ideally you would find a way to run the preprocessor more than once so you can double-expand (#if inside #define). If that doesn't work, at the very least, you can check the assertion condition and generate the error / message string only once, and then use the while pattern to use it in two different ways, depending on the assertions enabled state. To make things even clearer, you can even do something like #define LOG_ASSERTS !NS_ASSERTIONS_ENABLED so you don't keep re-typing that.

mgorbach commented 9 years ago

Many of your RZCAssert log statements appear to include self. That is a no-on and will cause painful problems with retain cycles.

mgorbach commented 9 years ago

This file is getting big (RZAssert.h). It might be a nice idea to pull it apart into logging components, shared components, and the actual assertions.

mgorbach commented 9 years ago

Check out NSException.h itself. It has recursive macros!

mgorbach commented 9 years ago

NSBlockAssertion is a mix of the implementations of NSCAssert and NSAssert. It formats the result like an assertion in an Objective C method, but does not capture self. NSCAssert also avoids capturing self, but it formats results as if you were in a C function (__PRETTY_FUNCTION__ instead of _cmd).

ZevEisenberg commented 9 years ago

@mgorbach this is ready to go (again). I did think of one possible final way to shorten the file:

#define RZASSERT_WITH_MESSAGE_LOG(expression, message, ...) \
    do { \
        RZASSERT_BASE( NO, @"**** Unexpected Assertion **** %@ \nExpression: \"%@\" \nSelf: \"%@\"", [NSString stringWithFormat:message, ##__VA_ARGS__], expression, self ) \
    } while(0)

This could be shortened to:

#define RZASSERT_WITH_MESSAGE_LOG(expression, message, ...)  RZASSERT_BASE( NO, @"**** Unexpected Assertion **** %@ \nExpression: \"%@\" \nSelf: \"%@\"", [NSString stringWithFormat:message, ##__VA_ARGS__], expression, self )

But it seemed like there would still be ways that that could break things if it’s not enclosed in a do/while.

mgorbach commented 9 years ago

This looks good, @ZevEisenberg. Just make the tweak to fully no-op assertions when logging is not activated and assertions are disabled, and you are clear to commit and release.

ZevEisenberg commented 9 years ago

@mgorbach this is ready. Travis seems to have given up for some reason, but you can run rake test yourself to see that tests are still working. be working again.