Many unnecessary operations in ISet collection

Issue #343 resolved
Jacek Laskowski created an issue

The ISet collection does a lot of unnecessary operations, I discovered it because my code is based on the log of these changes and it stopped working properly. Here is an minimal example:

program Intersect;

{$APPTYPE CONSOLE}

{$R *.res}

uses
  Spring,
  Spring.Collections,
  Spring.Collections.Sets,
  System.SysUtils;

type
 TTest = class
 public
   procedure OnChanged(sender: TObject; const item: Integer; action: TCollectionChangedAction);
   procedure Test();
 end;

procedure TTest.Test();
var
  lFirst, lSecond : ISet<Integer>;
begin
  lFirst := TCollections.CreateSet<Integer>([1, 2, 3]); //<- 
  lFirst.OnChanged.Add(OnChanged);
  lSecond := TCollections.CreateSet<Integer>([1, 2, 3]); //<- the same like previous
  lFirst.IntersectWith(lSecond);
end;

procedure TTest.OnChanged(sender: TObject; const item: Integer; action: TCollectionChangedAction);
begin
  Writeln('action: ', TEnum.GetName<TCollectionChangedAction>(action), '; value=', item);
end;

var
  Test : TTest;
begin
  try
    Test := TTest.Create;
    Test.Test();
    Test.Free;
    Readln;
  except
    on E: Exception do
      Writeln(E.ClassName, ': ', E.Message);
  end;
end.

After execute this code I get in console:

action: caRemoved; value=1
action: caRemoved; value=2
action: caRemoved; value=3
action: caAdded; value=1
action: caAdded; value=2
action: caAdded; value=3
action: caRemoved; value=1
action: caRemoved; value=1
action: caRemoved; value=1

Moreover... Did you not mistake Self and Sender in this definition? ;-)

  TCollectionBase<T> = class abstract(TEnumerableBase<T>)
  private type
    TNotify = procedure(Self: TObject; const item: T; action: TCollectionChangedAction);
  private

Comments (2)

  1. Stefan Glienke repo owner

    This is indeed a regression and that change I did there is embarassingly ridiculous I admit.

    My intention was to avoid the creation of a list but I also changed the algorithm in a bad way.
    I will fix this and add a test.

    As for the naming of the private TNotify - that is correct as this is just to eliminate the virtual call to Changed when it is not overridden and there is no event handler because then the fNotify is nil. Self becomes Sender when it is being passed to fOnChanged.Invoke. Declaring TNotify like that and not as procedure(const item: T; action: TCollectionChangedAction) of object;avoids having to store SizeOf(Pointer)*2 (which also affects the aligning of the class) but just SizeOf(Pointer) because Data is always Self anyway.

  2. Log in to comment