> I hope this change makes you happy!
It most certainly does! I'm a huge proponent of code cleanup and organization,
and this looks like a beautiful example of those.
> Finally, the root class, WidgetEvent, has As*Event class. The "*"
> doesn't include the prefix. I.e., for WidgetMouseEvent, the method name
> is WidgetEvent::AsMouseEvent(). This returns the pointer of the instance
> only when the instance is the class or its derived class. Otherwise,
> returns nullptr.
>
> E.g., WidgetDragEvent is defines as:
>
> WidgetEvent
> +- WidgetGUIEvent
> +- WidgetInputEvent
> +- WidgetMouseEventBase
> +- WidgetMouseEvent
> +- WidgetDragEvent
>
> If an instance is WidgetDragEvent, AsGUIEvent(), AsInputEvent(),
> AsMouseEventBase(), AsMouseEvent() and AsDragEvent() returns its
> pointer. The other As*Event() methods return nullptr.
>
> You should not use static_cast for Widget*Event and Internal*Event
> anymore because it may cause wrong casting bug with some mistakes
> (actually, there was such bug!).
I do have a question about this part. It seems that there are 3 distinct
situations where you might be casting pointers around in this inheritance tree.
1. Downcasting to a most-derived class (a leaf in the tree)
2. Downcasting to a non-leaf-node (i.e. a class that is not a most-derived
class)
3. Upcasting
For 1 and 3, I believe we can accomplish these safely using static_cast. For 3,
this is always true. For 1, I believe that the static_cast is safe as long as
you have checked the |eventStructType| member.
That is, the following should be safe:
WidgetWheelEvent *event1 = nullptr;
// Assume that aEvent is a WidgetEvent* that we received from elsewhere
if (NS_WHEEL_EVENT == aEvent->eventStructType) {
// We can now trust this cast
event1 = static_cast<WidgetWheelEvent*>(aEvent);
}
For 2, the |eventStructType| member by itself doesn't give us enough
information to know if a static_cast is safe (you'd have to keep a map of
most-derived classes to all of their parent classes... yuck). In these cases, I
feel that the As*Event functions are providing the same functionality as
dynamic_cast.
That is, the following are equivalent:
// Assume that aEvent is a WidgetEvent* that we received from elsewhere
WidgetInputEvent *event2 = aEvent->AsWheelEvent();
WidgetInputEvent *event3 = dynamic_cast<WidgetInputEvent*>(aEvent);
// At this point, |event2 == event3| should be true
So my question is this; is it preferable to keep the As*Event functions, or to
use regular C++ casts to accomplish the same behavior? We could use static_cast
unconditionally for upcasts, static_cast with a check of |eventStructType| for
downcasts to most-derived event types, and dynamic_cast for downcasts to
non-most-derived event types.
I don't think there would be a meaningful performance impact (case 3 would be
faster, case 2 would be a dynamic cast vs a virtual function call, case 1 would
be a branch vs a virtual function call), but I think it would clean up our
event types even further (again, great work cleaning them up this much!) and it
takes advantage of built-in support from the language to aid us in managing our
event types.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
