-
-
Notifications
You must be signed in to change notification settings - Fork 85
Update MpObjectReference.cpp #2387
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 3450ca9 in 1 minute and 34 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1179
- Draft comment:
Disabling std::terminate() may allow a critical error state to persist. Ensure this change is intentional and that the error is now handled appropriately. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid concern about error handling, but it's phrased as a speculative "may allow" and asks the author to "ensure" something. It's asking for confirmation rather than pointing out a clear issue. The comment doesn't provide a specific suggestion for how to handle the error state. The original code considered this a fatal error worthy of termination, so removing termination without an alternative error handling strategy could be problematic.
The comment identifies a potentially serious issue with error handling, which is important. Not having proper error handling could lead to undefined behavior.
While error handling is important, the comment violates our rules by asking for confirmation ("ensure") and being speculative ("may allow"). It doesn't provide actionable guidance.
The comment should be deleted because it asks for confirmation and is speculative rather than providing clear, actionable guidance about error handling.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1584
- Draft comment:
Consider renaming the method 'ActivateChilds' to 'ActivateChildren' for better clarity and to follow standard English conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_IhowvAER1Ow8YFVR
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| if (pImpl->setPropertyCalled) { | ||
| GetParent()->logger->critical("ApplyChangeForm called after SetProperty"); | ||
| std::terminate(); | ||
| // std::terminate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting out the termination without a fallback may leave the object in an inconsistent state. Consider handling the error properly.
Important
Comment out
std::terminate()inApplyChangeForminMpObjectReference.cppto prevent program termination when called afterSetProperty.std::terminate()inApplyChangeForminMpObjectReference.cpp, preventing program termination when called afterSetProperty.This description was created by
for 3450ca9. It will automatically update as commits are pushed.