On 2013/10/25 6:39, Tim Abraldes wrote:
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.
Please use As*Event() in case #1 and #2. As you said, we can use
static_cast since there is eventStructType. However, it's NOT safe
because we *might* take mistakes. Actually, there was such bug. If
somebody takes mistake, it may cause breaking memory if it sets value to
the member.
# Anyway, we don't need to do anything in case #3.
Additionally, the user code becomes simple since we can avoid checking
eventStructType member ;-)
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.
Sure. As*Event() approach has some advantages especially somebody who
works on event stuff. If somebody needs to change event class hierarchy,
it's difficult to find the static_casts which will be broken. However,
if As*Event() is used, being broken part cause crash due to accessing
nullptr. So, it may be detectable in automated tests.
The cost of using vtable must be enough cheap. Additionally, even if
it's not so, we can do same approach which we've taken in As*Event()
implementation.
BTW, I'm researching that if we can hide or remove eventStructType
completely. Checking eventStructType is doing very raw level work.
Therefore, it's causing new bugs when we add new event class. For
example, some code checks eventStructType for checking a class has if
WidgetMouseEventBase. If somebody add new subclass of it, he/she needs
to find and change such code. If he/she failed to find such code even
only one, the change would cause regression.
Although, I don't think that it causes problem when it checks only the
most derived class. Therefore, I think that creating Is*EventClass()
methods which *cannot* be used in switch statement is better for such
use case.
--
Masayuki Nakano <masay...@d-toybox.com>
Manager, Internationalization, Mozilla Japan.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
