Improve hsRef and simplify ref management in a few places#1872
Improve hsRef and simplify ref management in a few places#1872dgelessus wants to merge 16 commits into
hsRef and simplify ref management in a few places#1872Conversation
That is, call Ref/UnRef directly if it's clear from context that the pointer cannot be null - because it was already checked, or because it comes directly from new, or because the code path already assumes that the pointer is not null. This fixes some false positive warnings in CLion about missing null checks - CLion takes the null check inside the "safe" macros as an indication that the pointer can indeed be null, and then warns about any other uses of the same pointer variable without an explicit null check.
Hoikas
left a comment
Hoikas
left a comment
There was a problem hiding this comment.
I think this is fine, but I wonder if we should take this as an opportunity to use hsRef instead. There are several places where we have vectors of hsRefCnt and hsRefCnt members that we're manually performing the ref count management on, and I think changing those to hsRef would be a good code quality improvement.
| animMsg->AddCallback(eventMsg); | ||
| hsRefCnt_SafeUnRef(eventMsg); // AddCallback adds it's own ref, so remove ours (the default of 1) |
There was a problem hiding this comment.
There's quite a bit of this anti-pattern repeated. I wonder if it would make more sense to change AddCallback to accept an hsRef and change all of these call sites to create an hsRef of whatever message is being sent as the callback and std::move it as the callback. That's potentially a larger change though, and I don't think that should be a requirement.
There was a problem hiding this comment.
I'm wary of using hsRef with messages in particular, because then calling msg->Send() as usual will cause a double unref - you have to remember to use msg->SendAndKeep() instead. Which is unfortunate, because hsRef would definitely be useful for non-straightforward message fiddling code.
I wonder if we should add a dedicated plMessageRef class with its own Send method that automatically nulls out the ref after sending. Sadly, that still doesn't prevent you from calling msg->Send() (wrong) instead of msg.Send() (correct).
No longer using hsRefCnt - that was only needed to support sharing the global plAccessGeometry with external DLLs, which isn't used by anything. All non-global plAccessGeometry instances were already not refcounted.
It didn't set the kBCastByExactType flag.
And ensure that all fields are initialized. Previously, fCommand and fPageID were left uninitialized by the overloads without a corresponding parameter.
|
Yes, |
| // Remove ourselves from the stack | ||
| plInputIfaceMgrMsg *msg = new plInputIfaceMgrMsg( plInputIfaceMgrMsg::kRemoveInterface ); | ||
| msg->SetIFace( this ); | ||
| msg->SetIFace(hsRef<plInputInterface>(this)); |
There was a problem hiding this comment.
The implicit conversion from a raw pointer to hsRef doesn't seem to work if the raw pointer points to a subclass of the expected class. Adding an explicit constructor call like this fixes it for some reason (I'm calling exactly the type expected by the method).
There was a problem hiding this comment.
I fixed this by adding an explicit hsRef(_Ref* copy) constructor overload. That works even if the pointer type doesn't match exactly (unlike the indirect conversion via hsWeakRef that was previously used here).
dgelessus
changed the title
hsRefCnt_Safe[Un]Ref calls where possiblehsRef and simplify ref management in a few places
dgelessus
force-pushed
the
simplify_safe_refs
branch
from
3845898 to
e322cd9
Compare
3 participants
Replaces
hsRefCnt_Safe[Un]Refcalls by direct calls toRef/UnRefif it's clear from context that the pointer cannot be null - because it was already checked, or because it comes directly fromnew, or because the code path already assumes that the pointer is not null.This fixes some false positive warnings in CLion about missing null checks - CLion takes the null check inside the "safe" macros as an indication that the pointer can indeed be null, and then warns about any other uses of the same pointer variable without an explicit null check.