Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses potential null pointer crashes by adding checks after calls to CastingPlayer::GetTargetCastingPlayer(). The changes are correct and improve the robustness of the code. Storing the result of GetTargetCastingPlayer() in a local variable is also a good practice for both safety and efficiency. I've updated the suggestions to use the VerifyOrReturnError macro for improved conciseness and consistency with recommended practices for error handling, as indicated by the ChipLogError calls.
| if (targetCastingPlayer == nullptr) | ||
| { | ||
| ChipLogError(AppServer, | ||
| "CastingPlayer::VerifyOrEstablishConnection() Target CastingPlayer no longer exists, " | ||
| "skipping connection handling"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
While this null check is correct and crucial, for conciseness and consistency with other checks in this file (e.g., line 92), consider using the VerifyOrReturnError macro. This can be applied to the other null check added in this file as well. The use of ChipLogError indicates an error condition, making VerifyOrReturnError the more appropriate choice for early returns, as per recommended practice.
For example:
CastingPlayer * targetCastingPlayer = CastingPlayer::GetTargetCastingPlayer();
VerifyOrReturnError(
targetCastingPlayer != nullptr, CHIP_ERROR_INCORRECT_STATE,
ChipLogError(AppServer,
"CastingPlayer::VerifyOrEstablishConnection() Target CastingPlayer no longer exists, "
"skipping connection handling"));CastingPlayer * targetCastingPlayer = CastingPlayer::GetTargetCastingPlayer();
VerifyOrReturnError(
targetCastingPlayer != nullptr, CHIP_ERROR_INCORRECT_STATE,
ChipLogError(AppServer,
"CastingPlayer::VerifyOrEstablishConnection() Target CastingPlayer no longer exists, "
"skipping connection handling"));References
- Prefer using
VerifyOrReturnErrorfor early returns, even with negated conditions, as it is the recommended practice. An explanatory comment should be added to clarify the logic if it's not immediately obvious.
| if (targetCastingPlayer == nullptr) | ||
| { | ||
| ChipLogError(AppServer, | ||
| "ChipDeviceEventHandler::Handle() Target CastingPlayer no longer exists, skipping connection " | ||
| "handling"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This null check is a great addition for robustness. For better conciseness and consistency with the codebase's style, you could use the VerifyOrReturnError macro here. This suggestion also applies to the other null check added in this file. The use of ChipLogError indicates an error condition, making VerifyOrReturnError the more appropriate choice for early returns, as per recommended practice.
For example:
CastingPlayer * targetCastingPlayer = CastingPlayer::GetTargetCastingPlayer();
VerifyOrReturnError(targetCastingPlayer != nullptr, CHIP_ERROR_INCORRECT_STATE,
ChipLogError(AppServer,
"ChipDeviceEventHandler::Handle() Target CastingPlayer no longer exists, skipping connection "
"handling"));CastingPlayer * targetCastingPlayer = CastingPlayer::GetTargetCastingPlayer();
VerifyOrReturnError(targetCastingPlayer != nullptr, CHIP_ERROR_INCORRECT_STATE,
ChipLogError(AppServer,
"ChipDeviceEventHandler::Handle() Target CastingPlayer no longer exists, skipping connection "
"handling"));References
- Prefer using
VerifyOrReturnErrorfor early returns, even with negated conditions, as it is the recommended practice. An explanatory comment should be added to clarify the logic if it's not immediately obvious.
3 participants
Summary
fix crashes when not able to communicate with cached casting player
Testing
TBD