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.

Sign up for GitHub

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

Jump to bottom

Fixed analyze issue introduced in Xcode 12.5 #8208

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Fixed analyze issue introduced in Xcode 12.5 #8208

merged 2 commits into from
Jun 8, 2021

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Jun 7, 2021

Screen Shot 2021-06-07 at 7 12 51 AM
@google-cla google-cla bot added the cla: yes label Jun 7, 2021
@google-oss-bot
Copy link

google-oss-bot commented Jun 7, 2021 •
edited
Loading

Coverage Report

Affected SDKs

  • FirebaseDynamicLinks-iOS-FirebaseDynamicLinks.framework

    SDK overall coverage changed from 60.91% (ac7bcdc) to 76.63% (85bc6b5) by +15.72%.

    Filename Base (ac7bcdc) Head (85bc6b5) Diff
    FDLUtilities.m 73.48% 97.35% +23.86%
    FIRDLDefaultRetrievalProcessV2.m 14.06% 73.96% +59.90%
    FIRDLJavaScriptExecutor.m 26.28% 82.48% +56.20%
    FIRDynamicLink.m 80.17% 81.90% +1.72%
    FIRDynamicLinkNetworking.m 44.44% 81.72% +37.27%

Test Logs
@paulb777 paulb777 requested a review from eldhosembabu June 7, 2021 14:33
@paulb777 paulb777 enabled auto-merge (squash) June 7, 2021 18:35
@paulb777 paulb777 mentioned this pull request Jun 8, 2021
Merged
@paulb777 paulb777 merged commit 7dcf7e7 into master Jun 8, 2021
@paulb777 paulb777 deleted the pb-dl-analyze branch June 8, 2021 14:06
maksymmalyhin
maksymmalyhin approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM if we are sure that NULL is never passed as the errorPtr in a way that compiler and analyzer cannot catch. I would prefer to go with nullable pointer and a check for NULL
@paulb777
Copy link
Member Author

paulb777 commented Jun 8, 2021

Thanks. Confirmed that the caller allocates the error pointer - https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDynamicLinks/Sources/FIRDynamicLinkNetworking.m#L325
@paulb777 paulb777 mentioned this pull request Jun 22, 2021
Merged
@firebase firebase locked and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: dynamiclinks cla: yes
4 participants
@paulb777 @google-oss-bot @maksymmalyhin @ncooke3