Skip to content

kill debug assert and unnecessary macros - #3556

Merged
charlotteliang merged 6 commits into
masterfrom
fcm-kill-devAssert
Aug 13, 2019
Merged

kill debug assert and unnecessary macros#3556
charlotteliang merged 6 commits into
masterfrom
fcm-kill-devAssert

Conversation

@charlotteliang

@charlotteliang charlotteliang commented Aug 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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
morganchen12
morganchen12 reviewed Aug 12, 2019
View reviewed changes

@morganchen12 morganchen12 left a comment

Copy link
Copy Markdown
Contributor

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.

morganchen12
morganchen12 approved these changes Aug 12, 2019
View reviewed changes

@morganchen12 morganchen12 left a comment

Copy link
Copy Markdown
Contributor

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

charlotteliang commented Aug 12, 2019

Copy link
Copy Markdown
Member Author

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

@paulb777

paulb777 commented Aug 12, 2019

Copy link
Copy Markdown
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
@paulb777 paulb777 mentioned this pull request Aug 14, 2019
Merged
@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.

Reviewers

@paulb777 paulb777 paulb777 approved these changes
@morganchen12 morganchen12 morganchen12 approved these changes
@maksymmalyhin maksymmalyhin Awaiting requested review from maksymmalyhin

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@charlotteliang @paulb777 @morganchen12 @googlebot