38

After upgrading to XE8 some of our projects start to break data. Looks like a bug in TList realization.

program XE8Bug1;
{$APPTYPE CONSOLE}

uses
  System.SysUtils, Generics.Collections;

type
  TRecord = record
    A: Integer;
    B: Int64;
  end;

var
  FRecord: TRecord;
  FList: TList<TRecord>;

begin
  FList := TList<TRecord>.Create;
  FRecord.A := 1;
  FList.Insert(0, FRecord);
  FRecord.A := 3;
  FList.Insert(1, FRecord);
  FRecord.A := 2;
  FList.Insert(1, FRecord);
  Writeln(IntToStr(FList[0].A) + IntToStr(FList[1].A) + IntToStr(FList[2].A));

end.

This code prints "123" in XE7 and before (as it should be), but in XE8 it prints "120". Maybe someone know a quickfix for this?

Update: unofficial fix is here

BofA
  • 823
  • 7
  • 16
  • 9
    The generic collections have been re-implemented in XE8. Perhaps they don't have any unit tests at Emba. If this is as you describe, and it seems likely, your solution is to remain on XE7. You do need to submit a bug report. – David Heffernan Apr 27 '15 at 22:11
  • 3
    Reported as [Regression: TList.Insert not working](https://quality.embarcadero.com/browse/RSP-10773). – LU RD Apr 28 '15 at 04:56
  • 10
    So it seems that Embarcadero don't have an effective testing regime. How on earth could they have got this wrong? Such a fundamental class. A well run dev team would have unit tested this comprehensively. Such a fault should never get past that testing. Dismal. – David Heffernan Apr 28 '15 at 06:14
  • 6
    And I'd have to pay extra to get this (4-week old) product bug-fixed. – robsoft Apr 28 '15 at 07:47
  • 1
    Related: http://stackoverflow.com/questions/30850911/firemonkey-listview-item-indexes-not-updating I guess they figured that nobody needs to use `Insert` so they never tested it. – Jerry Dodge Jun 15 '15 at 18:59
  • @JerryDodge, fmx.TlistView seems to use TList internally with sizes of T > 8. I did not bother to see if any of them uses the `Insert` method though. You could trace with the debugger with a breakpoint at TListHelper.InternalInsertN if you like. – LU RD Jun 15 '15 at 20:46
  • This is fixed in XE8 upd 1, including some errors in TListHelper. – LU RD Jun 19 '15 at 11:10

1 Answers1

32

I found that now the TList<T>.Insert method call TListHelper.InternalInsertX depends on the data size, in my case:

procedure TListHelper.InternalInsertN(AIndex: Integer; const Value);
var
  ElemSize: Integer;
begin
  CheckInsertRange(AIndex);

  InternalGrowCheck(FCount + 1);
  ElemSize := ElSize;
  if AIndex <> FCount then
    Move(PByte(FItems^)[AIndex * ElemSize], PByte(FItems^)[(AIndex * ElemSize) + 1], (FCount - AIndex) * ElemSize);
  Move(Value, PByte(FItems^)[AIndex * ElemSize], ElemSize);
  Inc(FCount);
  FNotify(Value, cnAdded);
end;

I see the problem in the first Move call. Destination should be:

PByte(FItems^)[(AIndex + 1) * ElemSize]

not 

PByte(FItems^)[(AIndex * ElemSize) + 1]

Aaargh!

Finally, I've used the System.Generics.Defaults.pas and System.Generics.Collections.pas units from Delphi XE7 in my projects, and now all works as expected.

Update: as I see, RTL not affected, as it isn't use TList<T>.Insert for T with SizeOf > 8 (or maybe I miss something?)

BofA
  • 823
  • 7
  • 16
  • 3
    Fixing by restoring the XE7 implementation is not such a bad idea. You have more trust in it. Yes you can patch this one method, but don't you wonder what else they got wrong? – David Heffernan Apr 28 '15 at 06:11
  • 1
    Although if you replace the entire units, it would be wise to do so in a way that means the RTL/VCL/FMX etc. also use the fixed units. Otherwise you may be hurt by other manifestations of this defect. – David Heffernan Apr 28 '15 at 07:10
  • Mantra of the inexperienced developers: "why care to look at the old code, when you can write new code in a 'much better' way" ;-) (I have had too many of that kind of developers, and it is disappointing that Embarcadero have them at all) – Hans Apr 28 '15 at 08:26
  • 1
    There are many many places in the RTL where TList.Insert is used. How on earth could this have passed undetected? – LU RD Apr 28 '15 at 08:45
  • This bug persists only when SizeOf(T) > 8, so there's only a few places in RTL affected, most of them rounded with {$IFDEF POSIX} :) – BofA Apr 28 '15 at 09:00
  • 1
    @BofA `SizeOf(T)>8` **and** the type is unmanaged. Perhaps Emba get away with it by way of preferring dyn allocated classes to automatically allocated records. – David Heffernan Apr 28 '15 at 10:35