Check for override

[Sherlock 663]

11 hours ago, Dmitry Arefiev said:

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

FMX comes to mind. The first few versions rarely had anything in common other than the base language. But then...that was work in progress.

[Stefan Glienke 2002]

12 hours ago, Dmitry Arefiev said:

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

I am not going to search through JIRA right now but JSON Serialization and the years of aftershocks of Generics.Collections refactoring come to mind.

Or negative performance impact through changes in System.pas routines because of some feature that I might not even use (I remember some occasion where some dynamic array handling routine suddenly got severely slower)

 

I am not using FireMonkey myself but I know there were numerous of breaking changes - for the better probably but there it obviously was not such a problem to let the users bite the bullet and adapt.

 

Then some useful features in the IDE got missing during the theming rework (like additional context menu items in the editor tabs like open in explorer or showing the save state of those units)

 

Can probably find more examples by searching for "regression" in JIRA.

 

This is just a general overview of my feelings I have with every new version - crossing fingers nothing important broke. And this should not be.

 

[Rudy Velthuis 91]

On 3/18/2019 at 2:13 PM, Anders Melander said:

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

IRL backward compatibility matters.

Not always. Sometimes it is better to break things than to have to write code that has to parses the VMT to see if something was overridden. That is a pretty fragile piece of code. Again, I would have replaced it, not overloaded it.

[Rudy Velthuis 91]

On 3/18/2019 at 2:05 PM, Rudy Velthuis said:

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

Sherlock: why "Sad"? Breaking existing code is sometimes better than introducing such, er... "clever" (and fragile) tricks. Old code that used some FormatSettings needs an update too. They ne

[Rudy Velthuis 91]

On 3/18/2019 at 5:10 PM, Anders Melander said:

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

Really? I guess many projects also stranded because of subtle bugs caused by those backward-compatibility hacks. Yes, it is hard to maintain backward compatibility, and sometimes it makes sense. But other times it doesn't. And if it is hard, the chances are big you must resort to a hack like the one under discussion. In such cases, I rather see my code break, as long as it is clear that it breaks any why.

 

My code has broken often enough because of changes in the runtime, but usually, the alternative is not too complicated. So you take some time to fix, and, if you move your code to a new version, you'll have to test everything anyway, so that is no big deal and not much extra work.

[Rudy Velthuis 91]

On 3/18/2019 at 5:10 PM, Anders Melander said:

because it was simply to costly to locate and fix breaking changes

I don't understand. A breaking change like this (64 bit seek instead of 32 bit seek) would probably not even be noticed, or if it were, easily fixed. Most breaking changes, well, break things and thus stand out like a sore thumb. They are easy to find and easy to fix, IME.

[Stefan Glienke 2002]

For once I have to wholeheartedly agree with Rudy - I have to mark that on my calendar. 

[Anders Melander 1782]

22 minutes ago, Rudy Velthuis said:

I don't understand. A breaking change like this (64 bit seek instead of 32 bit seek) would probably not even be noticed, or if it were, easily fixed. Most breaking changes, well, break things and thus stand out like a sore thumb. They are easy to find and easy to fix, IME.

OK then, let me spell it out; In regular applications or even simple libraries individual braking changes are probably relatively easy to locate and fix. The TStream change for example could almost have been made with search/replace, but I guess they thought that the backward compatibility solution was safe enough that they didn't need to make it breaking. In retrospect, although I never personally got bitten by it, they should have marked the old overload as deprecated to flag that ppl should fix their code or else. FWIW I don't think the TStream method is fragile. From what I can see it's very robust.

 

For a large framework the situation can be different. A framework has an API and therefore a contract with the users of the framework. A breaking change in the RTL that lies below the framework can mean that the break propagates to the API of the framework. For example I just yesterday finished updating my local copy of a framework that sits on top of DevExpress from a two year old version of DevExpress to the latest version. DevExpress changed the type of some properties from TBitmap to TdxSmartGlyph and since the framework exposed those same properties as TBitmap it got stuck on that old version. If the framework had followed suit and also changed the type, then it again would have broken the code of hundreds of customers (the users of the framework) with no gain to them. The company that with this particular framework is still stuck on that old version of DevExpress since they no longer have have the in-house expertise to solve the problem and you can bet they would have preferred a non-breaking change.

 

Another example is systems with a plethora of applications and millions of LOC. A breaking change here can be hugely costly because of the accumulated time it takes to evaluate, fix and test each required change. In some cases the people that wrote the original code have moved on and nobody knows or understand what it does anymore. I see that daily (not the break, the lack of knowhow).

 

Anyway, I don't think there much point in flogging this much more so I'll let you have the last word - I know you want it 🙂

[Stefan Glienke 2002]

Funny that you bring up that DevExpress change - I remember upgrading our software and it was a non-issue. And in fact the "what's new" for that version explains exactly what to do.

Not saying any of us is right or wrong but you can see that something is a huge issue for one and none for another.

 

Yes, there is always some bad code that did not follow best practices or did not properly encapsulate something, leak an implementation detail or does something else making it a chore to move forward. But that is exactly why I wrote what I wrote 2 posts ago. You can always come up with an excuse (valid or not) to damn some breaking change. However sometimes you have to take the bitter pill to move forward painlessly either yourself or the library/framework developer that decided for such a change.

 

I for example will introduce quite some breaking changes for the next version of Spring4D and I experienced them myself when migrating a branch of our software to an early version and experienced all the required work. I even reverted some change because I saw that it was rather painful to find all the places and convert them easily. That means as a developer of a component/library/framework you should use that yourself in more than a toy project to get a feeling what consequences possible changes have to evaluate if they should be taken or not.

 

And it then is an important responsibility to document them and if necessary provide some tooling to migrate your code - I remember more than 10 years ago when moving from QuantumGrid 3 to 4 (I think) DevExpress provided a tool to convert all your code for that heavy breaking changes that came with that version change. If they would not have done that, I guess no existing customer would have done it - however I cannot tell about the problems that still existed, I joined the company when the change was done already.

Edited March 19, 2019 by Stefan Glienke

[Bill Meyer 337]

Breaking changes always come at a cost, and there will always be complaints. But few of us are prescient, and there comes a time -- as when DevExpress replaced the dxGrid with the cxGrid -- when the need to incur the breakage overpowers the costs. When an architecture proves too limiting, or too brittle, then carrying it forward is no kindness. But as Stefan wrote: "...it then is an important responsibility to document them and if necessary provide some tooling to migrate your code..."

[Sherlock 663]

14 hours ago, Rudy Velthuis said:

Sherlock: why "Sad"? Breaking existing code is sometimes better than introducing such, er... "clever" (and fragile) tricks.

"Sometimes" is a word I can relate to. So please consider this post a "consoled face" 😉

11 hours ago, Stefan Glienke said:

That means as a developer of a component/library/framework you should use that yourself in more than a toy project to get a feeling what consequences possible changes have to evaluate if they should be taken or not.

This is the essence: Don't we all have the feeling that Emborcagear is not really dogfooding? This gets obvious when the (FMX) example projects wont compile...something an automated script should pick up in the pre beta phase. The needed changes there could easily be expanded to a "How to cope with our latest breaking changes". And if that is not possible something is rotten in the state of Denmark.

[Rudy Velthuis 91]

On 3/19/2019 at 7:02 PM, Anders Melander said:

For a large framework the situation can be different. A framework has an API and therefore a contract with the users of the framework. A breaking change in the RTL that lies below the framework can mean that the break propagates to the API of the framework.

If they really wanted to keep the 32 bit Seek, they should not have overloaded it. They should have added a Seek64 method.

 

Or they could have omitted the 64 bit Seek that takes a Word as second parameter and only have introduced the one that takes an enumeration as second parameter. There would have been no ambiguity and no need for a hack. If they had deprecated the 32 bit Seek, all would have been fine (although I am not sure if the deprecated directive already existed, at that time).

 

And that is how you amend or improve large frameworks if backward compatbility is important. You don't resort to hacks. Of course, a Seek with a completely different signature would not have been a problem. The problem arises when the number of parameters is the same and they are assignmnet-compatible.

[Rudy Velthuis 91]

11 hours ago, Sherlock said:

Emborcagear

FWIW, I personally find this Emborcagear etc. stuff a little childish, just as childish as Micro$oft and similar name mutilations.

 

The name is Embarcadero. Not so hard to write, although Borland was easier. <g>

 

Borland and CodeGear were certainly dogfooding their stuff, at times, but certainly not all and certainly not always. I am not so sure about Embarcadero right now. Probably quite a lot of it, but certainly not everything. But even dogfooding is not a guarantee you'll find everything.

[Rudy Velthuis 91]

11 hours ago, Sherlock said:

This gets obvious when the (FMX) example projects wont compile

That's not dogfooding, that is plain (regression) testing.

 

Yes, they should do that, most definitely, even the examples. Especially for newcomers, non-working examples give a very bad expression.

[Remy Lebeau 1393]

21 hours ago, Rudy Velthuis said:

If they really wanted to keep the 32 bit Seek, they should not have overloaded it. They should have added a Seek64 method.

But then we would have been stuck with Seek64() moving forward, and much more code would have had to be re-written to migrate to it.

 

It is funny though, they have actually added a separate Seek32() now in 10.3 Rio, which takes a 32bit Integer for the Offset and a TSeekOrigin enum instead of a Word for the Origin parameter.  Wonder why they decided to do that after all these years, instead of just leaving the 32bit Seek() alone or remove it completely.

Quote

Or they could have omitted the 64 bit Seek that takes a Word as second parameter and only have introduced the one that takes an enumeration as second parameter.

That is exactly what they actually did originally when the 64bit Seek() was first added in Delphi 6.  The Word-taking version of the 64bit Seek() was added much later in XE4.  Why, I have no idea, especially since it was marked as 'deprecated' in the same version it was added in.

Quote

If they had deprecated the 32 bit Seek, all would have been fine (although I am not sure if the deprecated directive already existed, at that time).

The 'deprecated' directive was added in Delphi 6, the same version that added the first version of the 64bit Seek().

Edited March 21, 2019 by Remy Lebeau

[Rudy Velthuis 91]

51 minutes ago, Remy Lebeau said:

The 'deprecated' directive was added in Delphi 6, the same version that added the first version of the 64bit Seek().

Thanks. Then I would have made it deprecated.

 

51 minutes ago, Remy Lebeau said:

Why, I have no idea, especially since it was marked as 'deprecated' in the same version it was added in.

Perhaps someone was careless and deprecated the wrong one. <g>

Edited March 21, 2019 by Rudy Velthuis

[balabuev 102]

I see the topic is old already, but anyway, I'm searching for something faster. So, here is my try:

type
  TAncestor = class
    procedure ProcX; virtual;
    procedure DoSomething;
  end;

  TDescendantA = class(TAncestor) 
    procedure ProcX; override;
  end;

  TDescendantB = class(TAncestor) 
  end;

procedure TAncestor.DoSomething;
type
  TProcX = procedure of object;
var
  md: TProcX;
begin
  md := Self.ProcX;
  if TMethod(md).Code <> @TAncestor.ProcX then
    // Overridden.
  else
    // Not overridden.
end;

 

This is faster than previous solutions, but I don't like complicated long code (type declaration, md variable). Btw:

 

procedure TAncestor.DoSomething;
type
  TProcX = procedure of object;
var
  md: TProcX;
begin
  md := Self.ProcX; // Compiles.  
  md := TProcX(Self.ProcX); // Invalid cast :)).
  
  // And so:
  
  if TMethod(Self.ProcX).Code = ... then // Also invalid cast.
  	;
  
  if TMethod(TProcX(Self.ProcX)).Code = ... then // Also invalid cast.
  	;
    
  // Using "@Self.ProcX" does not work too...
end;

 

Edited April 8 by balabuev

[Remy Lebeau 1393]

13 hours ago, balabuev said:

I see the topic is old already, but anyway, I'm searching for something faster. So, here is my try:

procedure TAncestor.DoSomething;
type
  TProcX = procedure of object;
var
  md: TProcX;
begin
  md := Self.ProcX;
  if TMethod(md).Code <> @TAncestor.ProcX then
    // Overridden.
  else
    // Not overridden.
end;

 

Tested, works fine (also, you don't need to use "Self.").  You can also use an inline variable to simplify your example further:

procedure TAncestor.DoSomething;
begin
  var md := ProcX;
  if TMethod(md).Code <> @TAncestor.ProcX then
    // Overridden.
  else
    // Not overridden.
end;

Do note that in the original TStream code, using @TStream.Seek like above won't work since Seek() is overloaded, so the compiler wouldn't know which overload to compare with.  Hence the original code's use of a typed variable to resolve the ambiguity, eg:

procedure TAncestor.DoSomething;
type
  TProcX = procedure of object;
var
  Impl, Base: TProcX;
  ClassTAncestor: TClass;
begin
  ClassTAncestor := TAncestor;
  Impl := ProcX;
  Base := TAncestor(@ClassTAncestor).ProcX;
  if TMethod(Impl).Code <> TMethod(Base).Code then
    // Overridden.
  else
    // Not overridden.
end;

This works for both overloaded and non-overloaded methods.

 

I have reported this enhancement to Embarcadero now:

https://embt.atlassian.net/servicedesk/customer/portal/1/RSS-548

 

As for the invalid cast errors, they are probably the compiler trying to CALL the method first, and then cast the (lack of a) return value.  You have to do the comparisons through the TMethod record, and that requires a variable which you can type-cast.

 

Edited April 8 by Remy Lebeau

[balabuev 102]

On 4/8/2024 at 11:51 PM, Remy Lebeau said:

also, you don't need to use "Self.").  You can also use an inline variable to simplify your example further

Self is used for demo code clarity, of course it is not needed here. Yes, you right the code is shorter with inline variables (i don't have them in mind because I usually write libraries for XE2+, so my fail here).

 

On 4/8/2024 at 11:51 PM, Remy Lebeau said:

As for the invalid cast errors, they are probably the compiler trying to CALL the method first

Yes, the side effect of bracesless functions calls. Still it looks like inconsistency. By the way, as I mentioned, even "@ProcX" does not help.

 

On 4/8/2024 at 11:51 PM, Remy Lebeau said:

Do note that in the original TStream code, using @TStream.Seek like above won't work since Seek() is overloaded

Yes, you right. And the ClassTAncestor hack is needed because Seek is a virtual method, otherwise we can resolve overloads simpler:

Base := TStream(nil).Seek;