View Issue Details

IDProjectCategoryView StatusLast Update
0032871FPCRTLpublic2018-01-14 13:11
ReporterOndrej PokornyAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Product VersionProduct Build 
Target Version3.2.0Fixed in Version3.1.1 
Summary0032871: Make TStrings.LoadFromFile and .LoadFromStream Delphi-compatible
DescriptionRelated issue reports: 0029848, 0030508

Michael Van Canneyt made TStringStream constructor encoding-aware for 0030508 and broke FPC compatibility. The same scenario should be also applied for TStrings.LoadFromStream - now the LoadFromStream non-encoding overload is not encoding aware, which doesn't match Delphi behavior and recent TStringStream changes.

The attached patch makes TStrings.LoadFromStream(Stream: TStream) and TStrings.LoadFromFile(const FileName: string) encoding-aware.

The old-FPC compatible methods are LoadFromStreamRaw and LoadFromFileRaw.
TagsNo tags attached.
Fixed in Revision37965
FPCOldBugId
FPCTarget
Attached Files
  • TStrings-LoadFrom-Encoding-1.patch (1,907 bytes)
    Index: rtl/objpas/classes/classesh.inc
    ===================================================================
    --- rtl/objpas/classes/classesh.inc	(revision 37752)
    +++ rtl/objpas/classes/classesh.inc	(working copy)
    @@ -692,8 +692,10 @@
           AObject: TObject);
         procedure LoadFromFile(const FileName: string); overload; virtual;
         procedure LoadFromFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
    +    procedure LoadFromFileRaw(const FileName: string); virtual;
         procedure LoadFromStream(Stream: TStream); overload; virtual;
         procedure LoadFromStream(Stream: TStream; AEncoding: TEncoding); overload; virtual;
    +    procedure LoadFromStreamRaw(Stream: TStream); virtual;
         procedure Move(CurIndex, NewIndex: Integer); virtual;
         procedure SaveToFile(const FileName: string); overload; virtual;
         procedure SaveToFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
    Index: rtl/objpas/classes/stringl.inc
    ===================================================================
    --- rtl/objpas/classes/stringl.inc	(revision 37752)
    +++ rtl/objpas/classes/stringl.inc	(working copy)
    @@ -1003,7 +1003,21 @@
     
     
     
    -Procedure TStrings.LoadFromStream(Stream: TStream);
    +Procedure TStrings.LoadFromFileRaw(const FileName: string);
    +Var
    +        TheStream : TFileStream;
    +begin
    +  TheStream:=TFileStream.Create(FileName,fmOpenRead or fmShareDenyWrite);
    +  try
    +    LoadFromStreamRaw(TheStream);
    +  finally
    +    TheStream.Free;
    +  end;
    +end;
    +
    +
    +
    +Procedure TStrings.LoadFromStreamRaw(Stream: TStream);
     {
        Borlands method is no good, since a pipe for
        instance doesn't have a size.
    @@ -1042,6 +1056,12 @@
     end;
     
     
    +Procedure TStrings.LoadFromStream(Stream: TStream);
    +begin
    +  LoadFromStream(Stream, nil);
    +end;
    +
    +
     Procedure TStrings.LoadFromStream(Stream: TStream; AEncoding: TEncoding);
     {
        Borlands method is no good, since a pipe for
    
  • strings-loadfromfile-01.patch (773 bytes)
    Index: rtl/objpas/classes/classesh.inc
    ===================================================================
    --- rtl/objpas/classes/classesh.inc	(revision 37965)
    +++ rtl/objpas/classes/classesh.inc	(working copy)
    @@ -690,7 +690,7 @@
         procedure Insert(Index: Integer; const S: string); virtual; abstract;
         procedure InsertObject(Index: Integer; const S: string;
           AObject: TObject);
    -    procedure LoadFromFile(const FileName: string); overload;
    +    procedure LoadFromFile(const FileName: string); overload; virtual;
         procedure LoadFromFile(const FileName: string; IgnoreEncoding : Boolean);
         procedure LoadFromFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
         procedure LoadFromStream(Stream: TStream); overload; virtual;
    

Relationships

related to 0033021 resolvedMichael Van Canneyt 37962 has side effects for derived classes 

Activities

Ondrej Pokorny

2017-12-23 11:46

developer  

TStrings-LoadFrom-Encoding-1.patch (1,907 bytes)
Index: rtl/objpas/classes/classesh.inc
===================================================================
--- rtl/objpas/classes/classesh.inc	(revision 37752)
+++ rtl/objpas/classes/classesh.inc	(working copy)
@@ -692,8 +692,10 @@
       AObject: TObject);
     procedure LoadFromFile(const FileName: string); overload; virtual;
     procedure LoadFromFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
+    procedure LoadFromFileRaw(const FileName: string); virtual;
     procedure LoadFromStream(Stream: TStream); overload; virtual;
     procedure LoadFromStream(Stream: TStream; AEncoding: TEncoding); overload; virtual;
+    procedure LoadFromStreamRaw(Stream: TStream); virtual;
     procedure Move(CurIndex, NewIndex: Integer); virtual;
     procedure SaveToFile(const FileName: string); overload; virtual;
     procedure SaveToFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
Index: rtl/objpas/classes/stringl.inc
===================================================================
--- rtl/objpas/classes/stringl.inc	(revision 37752)
+++ rtl/objpas/classes/stringl.inc	(working copy)
@@ -1003,7 +1003,21 @@
 
 
 
-Procedure TStrings.LoadFromStream(Stream: TStream);
+Procedure TStrings.LoadFromFileRaw(const FileName: string);
+Var
+        TheStream : TFileStream;
+begin
+  TheStream:=TFileStream.Create(FileName,fmOpenRead or fmShareDenyWrite);
+  try
+    LoadFromStreamRaw(TheStream);
+  finally
+    TheStream.Free;
+  end;
+end;
+
+
+
+Procedure TStrings.LoadFromStreamRaw(Stream: TStream);
 {
    Borlands method is no good, since a pipe for
    instance doesn't have a size.
@@ -1042,6 +1056,12 @@
 end;
 
 
+Procedure TStrings.LoadFromStream(Stream: TStream);
+begin
+  LoadFromStream(Stream, nil);
+end;
+
+
 Procedure TStrings.LoadFromStream(Stream: TStream; AEncoding: TEncoding);
 {
    Borlands method is no good, since a pipe for

Thaddy de Koning

2017-12-23 13:11

reporter   ~0104967

Last edited: 2017-12-23 13:11

View 2 revisions

I think it is better to use a default like
     procedure LoadFromFile(const FileName: string; AEncoding: TEncoding = TEncoding.Default); overload; virtual;

That won't break things....

Michael Van Canneyt

2018-01-13 19:23

administrator   ~0105750

I have done this differently. Instead of introducing a new method, I added a
  IgnoreEncoding: Boolean = True
parameter to LoadFromStream/LoadFromFile.

The default "true" means that the old FPC way is still the default way for backward compatibility.

Ondrej Pokorny

2018-01-14 10:23

developer   ~0105779

Michael, your solution isn't good at all:

1.) This issue report is about Delphi-compatibility, which you didn't fix, so the issue report is not resolved. On the contrary, you changed parameters of a virtual method introducing yet another Delphi incompatibility.

2.) If you don't want to make TString.LoadFromStream(Stream: TStream) and LoadFromFile(const FileName: string) Delphi-compatible, better is not to change anything and close this report as "won't fix". Because the Encoding-aware overloads are Delphi-compatible: people needing code that works in FPC and Delphi the same way can use them. They cannot use LoadFromStream(MyStream, False) because Delphi doesn't have it.

3.) I still don't understand why you broke FPC-backwards compatibility to introduce Delphi-compatibility for TStringStream.Create but you don't want to do the same for TStrings.LoadFrom*. Are some FPC classes/methods prominent to keep them Delphi-incompatible but others not - even if they are analogous?

Michael Van Canneyt

2018-01-14 10:49

administrator   ~0105782

The whole encoding thing is a big huge mess.
 
The TStringStream cannot be done in a backwards compatible manner and a delphi compatible manner a the same time.

I will handle it, see the releated bugreport.

Ondrej Pokorny

2018-01-14 11:14

developer   ~0105785

> The whole encoding thing is a big huge mess.

It may look so for ANSI rtl. For UNICODE rtl you need it anyway. This brings a 4th point:

4.) You'll need the compatibility for UNICODE RTL anyway.

> The TStringStream cannot be done in a backwards compatible manner and a delphi compatible manner a the same time.

That applies to TStrings as well

Michael Van Canneyt

2018-01-14 11:18

administrator   ~0105786

OK. hopefully fixed better now.

Rationale: I didn't want to introduc a 'Raw' call, I preferred instead to introduce an (optional) boolean parameter to make the old way available.

Unfortunately, I forgot about descendents that override this call.

So now there is a new overloaded version with a mandatory boolean parameter, and the 'old' call calls this one, with a default of False, making the default behaviour Delphi compatible.

Whether or not this default is a good decision, only time will tell.

Please check and close if OK.

Ondrej Pokorny

2018-01-14 12:47

developer   ~0105787

It's OK now - just a small thing you probably missed - you deleted the virtual keyword for LoadFromFile(const FileName: string)

See patch.

+ Will you announce this rather big change on FPC mailing list?

Ondrej Pokorny

2018-01-14 12:47

developer  

strings-loadfromfile-01.patch (773 bytes)
Index: rtl/objpas/classes/classesh.inc
===================================================================
--- rtl/objpas/classes/classesh.inc	(revision 37965)
+++ rtl/objpas/classes/classesh.inc	(working copy)
@@ -690,7 +690,7 @@
     procedure Insert(Index: Integer; const S: string); virtual; abstract;
     procedure InsertObject(Index: Integer; const S: string;
       AObject: TObject);
-    procedure LoadFromFile(const FileName: string); overload;
+    procedure LoadFromFile(const FileName: string); overload; virtual;
     procedure LoadFromFile(const FileName: string; IgnoreEncoding : Boolean);
     procedure LoadFromFile(const FileName: string; AEncoding: TEncoding); overload; virtual;
     procedure LoadFromStream(Stream: TStream); overload; virtual;

Michael Van Canneyt

2018-01-14 13:06

administrator   ~0105788

Changed, documented in WIKI user changes page, and notified mailing lists.

Ondrej Pokorny

2018-01-14 13:11

developer   ~0105789

Thank you!

Issue History

Date Modified Username Field Change
2017-12-23 11:46 Ondrej Pokorny New Issue
2017-12-23 11:46 Ondrej Pokorny File Added: TStrings-LoadFrom-Encoding-1.patch
2017-12-23 12:05 Michael Van Canneyt Assigned To => Michael Van Canneyt
2017-12-23 12:05 Michael Van Canneyt Status new => assigned
2017-12-23 13:11 Thaddy de Koning Note Added: 0104967
2017-12-23 13:11 Thaddy de Koning Note Edited: 0104967 View Revisions
2018-01-13 19:23 Michael Van Canneyt Fixed in Revision => 37962
2018-01-13 19:23 Michael Van Canneyt Note Added: 0105750
2018-01-13 19:23 Michael Van Canneyt Status assigned => resolved
2018-01-13 19:23 Michael Van Canneyt Fixed in Version => 3.1.1
2018-01-13 19:23 Michael Van Canneyt Resolution open => fixed
2018-01-13 19:23 Michael Van Canneyt Target Version => 3.2.0
2018-01-14 10:23 Ondrej Pokorny Note Added: 0105779
2018-01-14 10:23 Ondrej Pokorny Status resolved => feedback
2018-01-14 10:23 Ondrej Pokorny Resolution fixed => reopened
2018-01-14 10:47 Michael Van Canneyt Relationship added related to 0033021
2018-01-14 10:49 Michael Van Canneyt Note Added: 0105782
2018-01-14 11:14 Ondrej Pokorny Note Added: 0105785
2018-01-14 11:14 Ondrej Pokorny Status feedback => assigned
2018-01-14 11:18 Michael Van Canneyt Fixed in Revision 37962 => 37965
2018-01-14 11:18 Michael Van Canneyt Note Added: 0105786
2018-01-14 11:18 Michael Van Canneyt Status assigned => resolved
2018-01-14 11:18 Michael Van Canneyt Resolution reopened => fixed
2018-01-14 12:47 Ondrej Pokorny Note Added: 0105787
2018-01-14 12:47 Ondrej Pokorny Status resolved => feedback
2018-01-14 12:47 Ondrej Pokorny Resolution fixed => reopened
2018-01-14 12:47 Ondrej Pokorny File Added: strings-loadfromfile-01.patch
2018-01-14 13:06 Michael Van Canneyt Note Added: 0105788
2018-01-14 13:06 Michael Van Canneyt Status feedback => resolved
2018-01-14 13:06 Michael Van Canneyt Resolution reopened => fixed
2018-01-14 13:11 Ondrej Pokorny Note Added: 0105789
2018-01-14 13:11 Ondrej Pokorny Status resolved => closed