Needless finalizing of virtual functions.

Development Programming & Scripting
,
1

I’d love for an engine dev to explain why some C++ functions (seemingly randomly) go out of their way to get overridden with final in a derived class when their base class specifically declared them as virtual so that they could be overridden?

For example, SWidget implements a nice virtual function for SetVisibility.

/* SWidget.h */

SLATECORE_API virtual void SetVisibility(TAttribute<EVisibility> InVisibility);

/* SWidget.cpp */
void SWidget::SetVisibility(TAttribute<EVisibility> InVisibility){
	VisibilityAttribute.Assign(*this, MoveTemp(InVisibility));
}

This is super useful and can be overridden if for example you need/want to run some logic prior to, or the visibility change occurring.

Then for no logical reason, in SCompoundWidget (commonly derived from) UE decided to finalize an override that just calls the base SWidget function… Why

/* SCompoundWidget.h */
	SLATECORE_API virtual void SetVisibility(TAttribute<EVisibility> InVisibility) override final;

/*SCompoundWidget.cpp*/
void SCompoundWidget::SetVisibility(TAttribute<EVisibility> InVisibility){
	SWidget::SetVisibility(MoveTemp(InVisibility));
}

I understand why one would choose to finalize a function, but more often than not, such as in SCompoundWidget there appears to be no good reason for it as the finalized override just calls the base implementation anyway.

I’m aware of the UUserWidget.OnVisibilityChanged event, but that only helps blueprint devs, and not CPP devs who may have needed their widget to inherit from UPanelWidget instead. Even then it’s only for UMG ‘post change’ events and not Slate.

The workaround would be to just or fix the engine source... or declare a SetVisibility_MySlateWidget function that does it’s work then calls the underlying function, but that just clutters the API and opens the door for maintenance and use errors.

This isn’t the first time I’ve encountered this design decision by the engine team for seemingly zero benefit either. As a minimum if finalize was actually needed/important they could comment why they ‘needed’ to block the function.

2

I doubt the developers who programmed this are out there somewhere. This function must have been written a very long time ago, so there probably isn’t anyone here who can explain exactly why it’s done this way. I checked the source code briefly, and it looks like all the child functions (except SNullWidget) just call the parent class function, so making it virtual doesn’t even make much sence to me, to be honest.

I think (and I could be wrong) that they made it final for all children because they thought overriding would never be necessary. A function, based on its name, should do exactly what it’s named for, and nothing more. Adding extra behavior to a function will actually create more confusion, since people won’t expect it to behave differently when overridden. If you want to add extra logic to it, you’ll probably have to create a new function and name it to reflect the new functionality. With the added logic, it won’t be just SetVisibility anymore, it will need a more proper name.

But these are just my thoughts. I think if I left the project and came back to it some time later, I wouldn’t even remember that I somehow modified SetVisibility function. If there is a bug (problem) in this function related to the new functionality, it will take me a while to figure it out. So I would say that actually overriding this function will most likely lead to maintenance and use errors (in the long run).