Check for override

[Jacek Laskowski 57]

Such a simple example with inheritance:

 

TAncestor = class
  procedure MethodX(); virtual
  procedure Do();
end;

TDescendantA = class(TAncestor) //override MethodX
  procedure MethodX(); override;
end;

TDescendantB = class(TAncestor) //no override MethodX
end;

procedure TAncestor.Do();
begin
 how to check if MethodX is overridden?  <<<<<<<<<<<<<<<<<<<<
end;

var
 A : TDescendatdA;
 B : TDescendantB;
begin
 A := TDescendatdA.Create;
 B := TDescendatdB.Create;
 A.Do();
 B.Do();

 

How to check if MethodX is overridden?

 

[Stefan Glienke 2019]

IsVirtualMethodOverride from Spring.VirtualClass.pas

 

procedure TAncestor.Do;
begin
  Writeln(IsVirtualMethodOverride(TAncestor, ClassType, @TAncestor.MethodX));
end;

 

Edited March 15, 2019 by Stefan Glienke

[Anders Melander 1815]

...or just see TStream.Seek(Longint, Word) in classes.pas.

[Remy Lebeau 1436]

To expand on Anders' comment - the TStream.Seek() method has two overloads, one for 32-bit seeks and one for 64-bit seeks.  By default, the 32-bit Seek() calls the 64-bit Seek() and vice versa.  Descendants must override one of the Seek() methods and the override must not call the inherited method.  To avoid an endless recursive loop, the 32-bit Seek() validates that the 64-bit Seek() method has been overridden before calling it.  This is the technique that it uses for that validation:

procedure TAncestor.Do();
type
  TMethodXType = procedure of object; // must match the signature of MethodX...
var
  Impl: TMethodXType;
  Base: TMethodXType;
  ClassTAncestor: TClass;
begin
  Impl := MethodX; // points to descendant's implementation of MethodX...
  ClassTAncestor := Self.ClassType; // get object's actual class type...
  while (ClassTAncestor <> nil) and (ClassTAncestor <> TAncestor) do // find the TAncestor base class...
    ClassTAncestor := ClassTAncestor.ClassParent;
  Base := TAncestor(@ClassTAncestor).MethodX; // points to TAncestor's implementation of MethodX...
  if TMethod(Impl).Code = TMethod(Base).Code then // which MethodX implementation is the object actually using?
  begin
    // MethodX is NOT overriden...
  end else begin
    // MethodX IS overriden...
  end;
end;

 

Edited March 15, 2019 by Remy Lebeau

[Jacek Laskowski 57]

Thanks for both solutions, I use Spring4D, but "seek" way is worth attention too (and probably faster).

[Remy Lebeau 1436]

1 hour ago, Jacek Laskowski said:

I use Spring4D, but "seek" way is worth attention too (and probably faster).

The Spring4D approach uses roughly the same principle - comparing the base method and derived method to see if they are located at the same memory address or not - but it goes about doing so in a very different and more intrusive manner.  It manually looks for the base method address in the base class's vtable to get the method's index within the vtable, and then it retrieves the address stored in the vtable of the derived class at the same index, and then compares the two addresses.  The Seek() approach is cleaner in that it lets the compiler deal with the vtable details.

[PeterBelow 239]

23 hours ago, Jacek Laskowski said:

Such a simple example with inheritance:

 

TAncestor = class
  procedure MethodX(); virtual
  procedure Do();
end;

TDescendantA = class(TAncestor) //override MethodX
  procedure MethodX(); override;
end;

TDescendantB = class(TAncestor) //no override MethodX
end;

procedure TAncestor.Do();
begin
 how to check if MethodX is overridden?  <<<<<<<<<<<<<<<<<<<<
end;

var
 A : TDescendatdA;
 B : TDescendantB;
begin
 A := TDescendatdA.Create;
 B := TDescendatdB.Create;
 A.Do();
 B.Do();

 

How to check if MethodX is overridden?

Well, you have been given some examples, but my question is: WHY do you want to know this? The whole point of polymorphism is that in the base class you do not need to know whether a descendant has overriden the method or not. In fact the base class does not even know whether there are descendents of it, and neither should it care. If you think you need to know whether a method has been overridden in descendents something is wrong with your class design, IMO.

23 hours ago, Jacek Laskowski said:

 

 

[Jacek Laskowski 57]

The class design is rather good, the override detection I need in order to optimize performance (class implements cache in the REST server).
The ancestor (in base class) method returns always False, the descendant method can change this behavior, but if it does not change, I prefer to check the boolean flag rather than call the method (just because it's faster).

[Rudy Velthuis 91]

3 hours ago, PeterBelow said:

If you think you need to know whether a method has been overridden in descendents something is wrong with your class design, IMO.

Indeed. I wonder a bit about the 32 bit/64 bit Seek in TStream too.

[Remy Lebeau 1436]

2 hours ago, Rudy Velthuis said:

Indeed. I wonder a bit about the 32 bit/64 bit Seek in TStream too.

It was done this way for backwards compatibility when TStream was first updated to support 64bit streams. Since all implementations and uses of TStream were based on 32bit operations at the time (to this day, some TStream implementations are still 32bit only - ahem, TMemoryStream), when the new 64bit Seek was added, nobody overrode it yet, so it would call the 32bit Seek by default so existing implementations would still work. Once a stream class was updated to override the 64bit Seek instead of the 32bit Seek, the 32bit Seek would then call the 64bit Seek by default, thus still maintaining compatibility with existing code. The 32bit Seek checking if the 64bit Seek is overriden before calling it is just a safety catch to avoid a recursive loop should an implementation make the mistake of not overriding either version of Seek correctly.

[Rudy Velthuis 91]

3 hours ago, Remy Lebeau said:

It was done this way for backwards compatibility when TStream was first updated to support 64bit streams. Since all implementations and uses of TStream were based on 32bit operations at the time (to this day, some TStream implementations are still 32bit only - ahem, TMemoryStream), when the new 64bit Seek was added, nobody overrode it yet, so it would call the 32bit Seek by default so existing implementations would still work. Once a stream class was updated to override the 64bit Seek instead of the 32bit Seek, the 32bit Seek would then call the 64bit Seek by default, thus still maintaining compatibility with existing code. The 32bit Seek checking if the 64bit Seek is overriden before calling it is just a safety catch to avoid a recursive loop should an implementation make the mistake of not overriding either version of Seek correctly.

If I had been them, I would simply have replaced Seek to accept a 64 bit parameter and to return a 64 bit value. I would have removed the 32 bit version. Versions only using 32 bit can still use those arguments. In other words: I would not have overloaded, just replaced.

[PeterBelow 239]

23 hours ago, Jacek Laskowski said:

The class design is rather good, the override detection I need in order to optimize performance (class implements cache in the REST server).
The ancestor (in base class) method returns always False, the descendant method can change this behavior, but if it does not change, I prefer to check the boolean flag rather than call the method (just because it's faster).

A clear case of premature optimization, IMO 😉

[Remy Lebeau 1436]

19 hours ago, Rudy Velthuis said:

If I had been them, I would simply have replaced Seek to accept a 64 bit parameter and to return a 64 bit value. I would have removed the 32 bit version. Versions only using 32 bit can still use those arguments. In other words: I would not have overloaded, just replaced.

Perhaps. It was so long ago, we don't know what their actual thinking was. Such a change would break existing implementations, forcing rewrites. It was probably the lesser of two evils at the time to introduce the overload to maintain compatibility rather than force people to update their code. 

 

Of course, the issue wasn't helped when a 3rd Seek overload was added in XE4, causing ambiguity errors under OSX/iOS 64bit for user code that was never updated to use the original overload added in D6.

 

Speaking from experience, I'm in a similar boat right now, where I have to update a legacy product with a huge codebase to migrate it from using 32bit ints to 64bit ints, and it is going to be a MAJOR undertaking, affecting databases and external APIs, and requiring rewrites of internal logic in various places.  Who would have thought 20 years ago that our customer data would ever need to exceed the limits of 10 digit integers, but it has for some time now and we have worked around the problem over the years by truncating data in various ways to maintain compatibility, instead of redesign the system. We finally looked at a redesign awhile ago, but all of our team members have been let go or moved to other teams before it was viable. Now a redesign is being looked at again and I'm the only one left to work on it.

 

So, it is not always an easy task to replace code rather than overload code.

[Anders Melander 1815]

20 hours ago, Rudy Velthuis said:

If I had been them, I would simply have replaced Seek to accept a 64 bit parameter and to return a 64 bit value. I would have removed the 32 bit version. Versions only using 32 bit can still use those arguments. In other words: I would not have overloaded, just replaced.

You're missing the point. The purpose was to provide backward compatibility for existing descendants of TStream - not existing code using TStream.

[Jacek Laskowski 57]

5 hours ago, PeterBelow said:

A clear case of premature optimization, IMO 😉

the temptation to optimize is sometimes so great... 😉

[Stefan Glienke 2019]

14 hours ago, Jacek Laskowski said:

the temptation to optimize is sometimes so great...

If the method you avoid to call would do heavy work... but even using the fastest approach to check if the method is overridden will be slower than just doing a virtual call on a method that just returns False.

And even if you perform that check only once and store it in a boolean field it's just not worth it imo.

Edited March 18, 2019 by Stefan Glienke

[Rudy Velthuis 91]

19 hours ago, Anders Melander said:

You're missing the point. The purpose was to provide backward compatibility for existing descendants of TStream - not existing code using TStream.

I would not have done that. I would have let the code break.

[Anders Melander 1815]

2 minutes ago, Rudy Velthuis said:

I would not have done that. I would have let the code break.

Good thing you're not in charge of the RTL then.

IRL backward compatibility matters.

[Dalija Prasnikar 1404]

33 minutes ago, Anders Melander said:

Good thing you're not in charge of the RTL then.

IRL backward compatibility matters.

YES, backward compatibility matters. But in cases where backward compatibility causes more trouble down the road, then it is not worth the price.

 

In this case, maintaining backward compatibility also opened TStream and descendant classes to subtle bugs when working with streams larger than 2GB. 

[Anders Melander 1815]

6 minutes ago, Dalija Prasnikar said:

YES, backward compatibility matters. But in cases where backward compatibility causes more trouble down the road, then it is not worth the price.

 

In this case, maintaining backward compatibility also opened TStream and descendant classes to subtle bugs when working with streams larger than 2GB. 

Are you saying that it wasn't worth it back when the change was made or that it isn't worth it anymore (i.e. today)?

IMO the change was definitely worth it at the time because it didn't break backward compatibility and AFAIR didn't introduce new problems. They could have marked the old methods deprecated at some point and eventually retired it completely.

AFAIK the change didn't introduce any new problems in older TStream descendants with 2+Gb files - it just made them possible going forward. If you have examples of bugs then I'd love to hear of them.

[Dalija Prasnikar 1404]

That change was not worth while even back then. You had two variants for Seek and it was very easy to use wrong one. If you used 32 bit one on streams that support 64 bit your code would only work for streams smaller than 2GB. 

Change didn't affected older code, but it had permanent negative effect on new code.

 

I encountered such bugs myself, and also have seen others bumping into it on numerous occasions. Yes, at the end our code was at fault, but it was really easy mistake to make. 

 

There was several bug reports around that issue at the time in old Quality Central. 

[Stefan Glienke 2019]

"Backwards compatibility" is the ultimate excuse to pile up garbage in your backyard ...

 

It is used or ignored whenever convenient - moving forward also includes getting a compile error in your face but with a clear guide at hand how to solve it.

If you ever inherited from a TDataSet and used one of its method that have TBookmark or TRecordBuffer arguments while writing code for different Delphi versions since 2010 or so you know what I mean.

 

But some developers seem to rather want to save an hour when moving their code to a new version and waste hours or days later hunting down a bug. 😉  

Edited March 18, 2019 by Stefan Glienke

[Anders Melander 1815]

1 hour ago, Stefan Glienke said:

"Backwards compatibility" is the ultimate excuse to pile up garbage in your backyard ...

 

It is used or ignored whenever convenient - moving forward also includes getting a compile error in your face but with a clear guide at hand how to solve it.

If you ever inherited from a TDataSet and used one of its method that have TBookmark or TRecordBuffer arguments while writing code for different Delphi versions since 2010 or so you know what I mean.

 

But some developers seem to rather want to save an hour when moving their code to a new version and waste hours or days later hunting down a bug. 😉  

Ehem.. I think you should speak for yourself and not put these labels on the motives of other developers you know nothing about.

Breaking backward compatibility is easy. Maintaining it is often very hard.

I have seen many examples of projects that have stranded on old versions of 3rd party libraries because it was simply to costly to locate and fix breaking changes. That said I naturally agree that one should strive to move code forward and eliminate dependencies on backward compatibility.

[Stefan Glienke 2019]

11 minutes ago, Anders Melander said:

I think you should speak for yourself ...

I did exactly that.

 

12 minutes ago, Anders Melander said:

stranded on old versions of 3rd party libraries because it was simply to costly to locate and fix breaking changes.

And that is why I wrote "a clear guide at hand how to solve it" - breaking changes without a migration guide are bad.

 

I have seen many occasions where Borgearcadera justified not fixing bugs with "backwards compatibility" to keep broken code working and push the need to keep writing code in a broken way (design wise or literally) or added ridiculous workarounds into the code to fix something but without the necessity for anyone to fix their code - and I am not talking about subtle and hard to track things.

 

But then on the other hand break things every other version just because...

[Dmitry Arefiev 106]

3 hours ago, Stefan Glienke said:

But then on the other hand break things every other version just because... 

 Could you please provide some tickets to better understand what and where.