Ask for comments to improve this code

[Berocoder 14]

This is the code for Supports method. I know the name is bad, SysUtils.Supports is different. But we use this on thousands of places. Too late to change now.

Now I also use Pascal Analyzer. That give a warning STW10

Out parameter is read before set (STW10)

 

This section lists out parameters that are read before set in the procedure.

https://www.peganza.com/PALHelp/clip0211.png
 

And yes it is correct. First line

  TObject(ResObj) := nil;
 

Read the out parameter before set it.
ResObj have no class in parameter. Reason is that we want to send any subclass derived from TBoldObject.
If I set type to TObject that would not work any more.

Any suggestion how I can remove the warning and still preserve the implementation ?

 

{: Casting between two objects derived from two different classes
Resobj must inherit from aObject
@param aObject The object to cast from
@param aClass The class to cast to
@ResObj The resulting object
@return True if successful otherwise false}
function Supports(const aObject: TObject; const aClass: TClass; out ResObj): Boolean; overload;
begin
  TObject(ResObj) := nil;
  if aObject is aClass then
  begin
    if not ((aObject is TBoldObject) and (aObject as TBoldObject).BoldObjectIsDeleted) then
      TObject(ResObj) := aObject;

  end;
  result := TObject(ResObj) <> nil;
end;

Example of usage
 

var myInvoice: TInvoice;   // This inherit TBoldObject
if Supports(myclass.somelink, TInvoice, myInvoice)
begin
  // code that use myInvoice
end;

 

[David Heffernan 2345]

Pascal Analyser is wrong. Seems pointless to test with is TBoldObject and then cast with as. Once is tells you the cast is valid then go ahead and use an unsafe cast. 

 

Seems dubious to cast one object to be a class from an unrelated hierarchy. Don't know how that could be valid. 

 

Also, what is BoldObjectIsDeleted all about? Don't tell me this flag is set to true when the instance is destroyed. 

[Attila Kovacs 629]

Wow.

Thinking out loud: If you already cast ResObj to TObject in the code, can't you just give it the same type in the parameter list?

Edited August 9, 2022 by Attila Kovacs

[Uwe Raabe 2057]

Which version of Pascal Analyzer do you use? I couldn't reproduce it here.

[Uwe Raabe 2057]

6 minutes ago, Attila Kovacs said:

If you already cast ResObj to TObject in the code, can't you just give it the same type in the parameter list?

That would require to pass a TObject variable to that parameter, which is not helpful. An out parameters works like a var parameter, where the types must match exactly.

[Attila Kovacs 629]

1 minute ago, Uwe Raabe said:

That would require to pass a TObject variable to that parameter, which is not helpful. An out parameters works like a var parameter, where the types must match exactly.

I see.

[Uwe Raabe 2057]

Thinking out of the box here:

type
  TSupport = class helper for TObject
  public
    function Supports<T: class>(out ResObj: T): Boolean;
  end;

function TSupport.Supports<T>(out ResObj: T): Boolean;
begin
  ResObj := nil;
  if Self is T then
  begin
    if not ((Self is TBoldObject) and TBoldObject(Self).BoldObjectIsDeleted) then
      ResObj := T(Self);
  end;
  result := ResObj <> nil;
end;

 

[Berocoder 14]

10 hours ago, David Heffernan said:

Pascal Analyser is wrong. Seems pointless to test with is TBoldObject and then cast with as. Once is tells you the cast is valid then go ahead and use an unsafe cast. 

 

Seems dubious to cast one object to be a class from an unrelated hierarchy. Don't know how that could be valid. 

 

Also, what is BoldObjectIsDeleted all about? Don't tell me this flag is set to true when the instance is destroyed. 

Yes, BoldObjectIsDeleted is true when object is deleted in memory but remain in database. On next sync memory to database it will be removed from database as well.
But how that works is unrelated to question in this case.

[Berocoder 14]

8 hours ago, Uwe Raabe said:

Thinking out of the box here:

type
  TSupport = class helper for TObject
  public
    function Supports<T: class>(out ResObj: T): Boolean;
  end;

function TSupport.Supports<T>(out ResObj: T): Boolean;
begin
  ResObj := nil;
  if Self is T then
  begin
    if not ((Self is TBoldObject) and TBoldObject(Self).BoldObjectIsDeleted) then
      ResObj := T(Self);
  end;
  result := ResObj <> nil;
end;

Hm interesting idea 😀 Thanks!
But that change how call the method. How will that look like ?

 

Quote

 

 

Edited August 10, 2022 by Berocoder

[David Heffernan 2345]

12 minutes ago, Berocoder said:

But how that works is unrelated to question in this case.

It sounded like you might be accessing an object after it has been destroyed which is obviously incorrect. 

[Uwe Raabe 2057]

2 hours ago, Berocoder said:

But that change how call the method. How will that look like ?

procedure TForm1.SomeControlEvent(Sender: TObject);
begin
  var myLabel: TLabel;
  if Sender.Supports<TLabel>(myLabel) then 
    myLabel.Caption := 'handled';
end;

 

[ventiseis 2]

On 8/9/2022 at 11:19 PM, Uwe Raabe said:

 

function Supports<T: class>(out ResObj: T): Boolean;

 

 

This looks very good (and works)!

But please note that in the compiled result you will get an instance of this function for every type <T> you use it for - even if it likely does the same for all TObjects which you pass in. 

If you have a lot of types and a lot of utility functions like this, this will increase your executable file.

[David Heffernan 2345]

2 hours ago, ventiseis said:

 

This looks very good (and works)!

But please note that in the compiled result you will get an instance of this function for every type <T> you use it for - even if it likely does the same for all TObjects which you pass in. 

If you have a lot of types and a lot of utility functions like this, this will increase your executable file.

True, but so what? If you want slim executables you wouldn't be using Delphi at all.

[David Champion 48]

On 8/9/2022 at 7:24 PM, Berocoder said:

This is the code for Supports method. I know the name is bad, SysUtils.Supports is different. But we use this on thousands of places. Too late to change now. 

Dont give up on better code :-). Migrate the name and method signature slowly to something better. Use the Deprecated key word and introduce the replacement.

Edited August 12, 2022 by David Champion

[Berocoder 14]

On 8/10/2022 at 9:01 AM, David Heffernan said:

It sounded like you might be accessing an object after it has been destroyed which is obviously incorrect. 

It is correct. Deleted is not the same as destroyed.
A BoldObject is usually persisted in a database.

Summary of lifecycle of such object:

• Some code ask for the object. It is loaded from database to memory. A new instance in memory is created.
• It can now be used as any normal object.
• Some code call Delete method. The flag above BoldObjectIsDeleted is now true
• Some code Synchronize memory with database. Objects that are changed will update rows in database.
  Objects that are deleted in memory will also be deleted in database.
• The object will now be destroyed

 

Edited August 15, 2022 by Berocoder

[ventiseis 2]

On 8/11/2022 at 5:20 PM, David Heffernan said:

True, but so what? If you want slim executables you wouldn't be using Delphi at all.

It is not about small executables. It's about the delphi IDE which becomes which becomes unresponsive and suffers from long compile and link times (including cases of EOutOfMemory). For example, we have an internal library using derived generic TList class. And we have a lot of business model objects stored in this list class. For each and every generic list object type, all methods of TList are compiled again into the executable, even if they're doing the same thing. Perhaps with some trickery one would be able to replace this list class with a non-generic version.

I only wanted to add the comment that it is nice to have clean and working generic code but that it has a downside, too.

[Fr0sT.Brutal 900]

17 minutes ago, ventiseis said:

For example, we have an internal library using derived generic TList class. And we have a lot of business model objects stored in this list class. For each and every generic list object type, all methods of TList are compiled again into the executable, even if they're doing the same thing

You better derive TList<TObj> from classic TList, only overriding Get/Put methods so only those slim overrides would be compiled for each type used.

type
  TTest = class
    fobj: TObject;
  end;

  TTest<T: class> = class(TTest)
    function get: T;
  end;

function TTest<T>.get: T;
begin
  Result := fobj as T;
end;

var
  test_tform: TTest<TForm>;

 

[David Heffernan 2345]

2 hours ago, ventiseis said:

It is not about small executables. It's about the delphi IDE which becomes which becomes unresponsive and suffers from long compile and link times (including cases of EOutOfMemory). For example, we have an internal library using derived generic TList class. And we have a lot of business model objects stored in this list class. For each and every generic list object type, all methods of TList are compiled again into the executable, even if they're doing the same thing. Perhaps with some trickery one would be able to replace this list class with a non-generic version.

I only wanted to add the comment that it is nice to have clean and working generic code but that it has a downside, too.

I'm not convinced by this. But then I was primarily reacting to what you wrote. 

 

On 8/11/2022 at 2:24 PM, ventiseis said:

 

If you have a lot of types and a lot of utility functions like this, this will increase your executable file.

 

[Stefan Glienke 2002]

People overuse generics and forget that the implementation often does not need generics at all:

type
  TSupport = class helper for TObject
  private
    function __Supports(out ResObj: TObject; cls: TClass): Boolean;
  public
    function Supports<T: class>(out ResObj: T): Boolean; inline;
  end;

function TSupport.__Supports(out ResObj: TObject; cls: TClass): Boolean;
begin
  if Assigned(Self) and Self.InheritsFrom(cls)
    and not ((Self.InheritsFrom(TBoldObject)) and TBoldObject(Self).BoldObjectIsDeleted) then
  begin
    ResObj := Self;
    Exit(True);
  end;
  ResObj := nil;
  Result := False;
end;

function TSupport.Supports<T>(out ResObj: T): Boolean;
begin
  Result := __Supports(TObject(ResObj), T);
end;

This code will not cause any bloat at all (in the final executable - see below for the situation during compilation though).

 

On 8/16/2022 at 3:43 PM, David Heffernan said:

I'm not convinced by this. But then I was primarily reacting to what you wrote. 

What @ventiseis wrote is correct - the issues with long compile-time and possibly running ouf of memory (more so under the LLVM based compiler because they have a larger memory usage already) are real.

They are caused by the compiler first compiling unit by unit and stuffing everything generic being used into each dcu just to (possibly) remove it again later during link time. Linker however will only remove exact identical duplicates such as if you have TList<TFoo> in multiple units executable will only contain it once, but it will also contain TList<TBar> and TList<TQux> even though they are also just lists for objects. Since XE7 most methods on TList<T> are marked as inline and unless you are using runtime packages they are not being called in your executable but the internal TListHelper ones but since all RTL classes have $RTTI turned on the binary will still contain all these methods. You only partially get rid of them by using {$WEAKLINKRTTI ON} which you need to put into each and every unit (years ago you could put that into the dpr and it affected all units but that was actually a compiler bug that made flags with scope local being global - but any unit that changed them then also changed them globally which caused quite some weird defects)

That is why I did the huge refactoring in Spring4D regarding collections and the generic container registration API. If you use a list from spring with a thousand different classes the binary will only contain the code for the list once unlike with the RTL where you would have it a thousand times

 

The issues are known and reported but obviously are not severe enough or too complex (especially the second one would require some architectural changes to how the compiler deals with generics):

https://quality.embarcadero.com/browse/RSP-16520

https://quality.embarcadero.com/browse/RSP-18080

Edited August 18, 2022 by Stefan Glienke