Skip to content

Handle passing null to cpp instance manager #348

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

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

a-maurice
Copy link
Collaborator

Description

Provide details of the change, and generalize the change in the PR title above.

The internal GetInstance classes are sometimes expected to return null, for example Storage does if passing an invalid bucket. Instead of checking for null in each case, it seems better to let the cpp_instance_manager handle when null is passed in, especially instead of storing those nulls, as it currently does, assuming assertions are off.


Testing

Describe how you've tested these changes.

Building and running locally


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

@a-maurice a-maurice requested a review from jonsimantov June 8, 2022 21:35
Copy link
Contributor

@jonsimantov jonsimantov left a comment

Choose a reason for hiding this comment

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

Do we check the result anywhere?

@a-maurice
Copy link
Collaborator Author

Do we check the result anywhere?

Only place I saw was in the test file (that is currently not used).

@a-maurice a-maurice merged commit 704bc54 into main Jun 8, 2022
@a-maurice a-maurice deleted the am-instance_handle_null branch June 8, 2022 21:50
@firebase firebase locked and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants