Jump to content
Sign in to follow this  
Stefan Glienke

Cast error when using Ctrl+#

Recommended Posts

The code in TGotoModificationBaseExpert.HandleOnMenuPopup is causing an error every time I use Ctrl+# in 10.4.1

 

That is because there is an unconditional soft cast of _Sender to TPopupMenu which is not true all the time. Adding if _Sender is TPopupMenu around it fixes it.

Edited by Stefan Glienke

Share this post


Link to post

WTF? It's a bloody TMenuItem! That doesn't even have a common ancestor with TPopupMenu (apart from TComponent)! Who in their right mind would do that?

 

Fixed in revision #3325

  • Thanks 1

Share this post


Link to post

Unconditional cast should never be used on UI/Client side. Always uses "is" or "as".
It may be used only when performance really matters, e.g. on core server code, with a large test coverage, and only on proven bottlenecks - which are very rare.

The first is to write "as" to raise an exception. Then - maybe - use direct class casting. Only if really needed.

 

IMHO such direct class casting - or cascaded "if .. is .. then ... else if .. is .. then ..." may be a smell for potential breaking some of the SOLID principles.
In VCL events, we use Sender: TObject most of the time - it could be seen as a design flaw / legacy debt. But it is a reality.
We should be able to avoid direct class casting in most of our modern code.

Edited by Arnaud Bouchez

Share this post


Link to post
2 hours ago, Arnaud Bouchez said:

Unconditional cast should never be used on UI/Client side. Always uses "is" or "as".
It may be used only when performance really matters, e.g. on core server code, with a large test coverage, and only on proven bottlenecks - which are very rare.

The first is to write "as" to raise an exception. Then - maybe - use direct class casting. Only if really needed.

 

IMHO such direct class casting - or cascaded "if .. is .. then ... else if .. is .. then ..." may be a smell for potential breaking some of the SOLID principles.
In VCL events, we use Sender: TObject most of the time - it could be seen as a design flaw / legacy debt. But it is a reality.
We should be able to avoid direct class casting in most of our modern code.

Not sure what you are trying to tell us. The code in question was:

procedure TGotoModificationBaseExpert.HandleOnMenuPopup(_Sender: TObject);
var
  pm: TPopupMenu;
begin
  pm := _Sender as TPopupMenu;
  if Assigned(FOrigPopupEvent) then begin
    FOrigPopupEvent(_Sender);
  end;
  AppendMenuItem(pm);
end;

So this already was an 'as' operation which bombed out in Delphi 10.4 because somehow a TMenuItem was passed as Sender.

 

The new code looks like this:

procedure TGotoModificationBaseExpert.HandleOnMenuPopup(_Sender: TObject);
begin
  if Assigned(FOrigPopupEvent) then begin
    FOrigPopupEvent(_Sender);
  end;
  if _Sender is TPopupMenu then begin
    // sometimes Delphi 10.4.1 passes a TMenuItem here, no idea how this can happen, but it caused a runtime error
    AppendMenuItem(TPopupMenu(_Sender));
  end;
end;

So now I first call the original OnPopup-event and then I check whether the Sender is really a TPopupMenu before adding an item to it.

 

I can't see how this relates to what you wrote. There was no hard unconditional cast (OK, now there is, but only after an 'is'). There is no if .. is .. then else if .. is .. then.

Share this post


Link to post

My remark was a general one. It was not about this explicit code.

Writing `TPopupMenu(_Sender)` is a hard-cast which should never appear in most code, unless you have a very vague/legacy event like a TNotifyEvent. The VCL requires it.
 

For UI code, I would write:

 if _Sender is TPopupMenu then begin
    // sometimes Delphi 10.4.1 passes a TMenuItem here, no idea how this can happen, but it caused a runtime error
    AppendMenuItem(_Sender as TPopupMenu);
  end;

The code is somewhat redundant, because "is" is actually doing the class hierarchy check that "as" is doing: calling internally InheritsFrom() twice
But it is only slightly slower - unnoticeable on a VCL app, since passing a Windows message will be slower than this "as" execution.
IMHO it is safer and easier to read and to maintain.

Having used "as" at first hand did indeed give a hint about the initial problem.

 

Hardcasting is like premature optimization on client side. 

Edited by Arnaud Bouchez

Share this post


Link to post

Ctrl+# is the toggle comment menuitem on the editors context menu in the german IDE, not sure though if it has anything to do with whatever it is, but I'm sure you already checked where this "Sender" belongs to.

Share this post


Link to post
8 hours ago, Attila Kovacs said:

Ctrl+# is the toggle comment menuitem on the editors context menu in the german IDE, not sure though if it has anything to do with whatever it is, but I'm sure you already checked where this "Sender" belongs to.

I tried, but the call stack didn't give any clue where this call comes from. I guess the IDE is calling that event explicitly as MyPopup.OnPopup(Whatever) somewhere and the parameter isn't used in the original event implementation so it doesn't care what it is. The MenuItem neither has a name nor a caption, which is odd.

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  
×