Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kill debug assert and unnecessary macros #3556

Merged
merged 6 commits into from Aug 13, 2019
Merged

Conversation

charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Aug 12, 2019

Remove _FIRMessagingDevAssert that are only called if a DEBUG flag, which is never set in both debug/production build. Most of the assert logics are already been checked in the code, for the ones that are not, we replace it with a debugLog.
Also remove a few macros that are either not used at all or barely used.

@charlotteliang charlotteliang changed the title kill debug code kill debug assert and unnecessary macros Aug 12, 2019
Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing all or some of these asserts with warning logs.

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please wait for core approval before merging.

@charlotteliang
Copy link
Contributor Author

Good catch! I'm adding more debug/warning logs.

@paulb777
Copy link
Member

Looks like real failures in travis:

Failing tests:
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]

@charlotteliang charlotteliang merged commit eade6bd into master Aug 13, 2019
@charlotteliang charlotteliang deleted the fcm-kill-devAssert branch August 13, 2019 21:12
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants