View Issue Details

IDProjectCategoryView StatusLast Update
0031985FPCCompilerpublic2018-01-14 10:28
ReporterOndrej PokornyAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version3.1.1Product Build36212 
Target Version3.2.0Fixed in Version3.1.1 
Summary0031985: TWriter doesn't stream stored empty strings
DescriptionI'd like to reopen the issue 0020166.

There is absolutely no possibility to make the TWriter stream an empty string:

1.) The "nodefault" modifier doesn't work (contrary to Michael's statement "If you want a value to be written regardless, you must append the nodefault modifier to the property definition, this will cause TWriter to ignore default values."
2.) A stored method returning True doesn't work.
3.) An explicit "stored True" doesn't work.

As a result, if a component uses a non-empty default string value (e.g. TMyComp.S from the example) and you change the value to empty string => the property isn't written into the stream. A subsequent read doesn't restore the empty string but the default string, obviously.
Steps To ReproduceSee the attached test case project. All string properties S, T and U are explicitly told to be written to the stream (every one with a different approach). None of them is written.

Compare it to the Integer properties I, J, K with the same modifiers - all of them are stored.
Additional InformationA possible (simple) patch is attached. It copies the Integer/Ord approach. The nodefault modifier isn't handled, though - neither by ordinal properties, nor by string properties.
TagsNo tags attached.
Fixed in Revision37954
FPCOldBugId
FPCTarget
Attached Files
  • writer-empty-string-1.patch (975 bytes)
    Index: rtl/objpas/classes/writer.inc
    ===================================================================
    --- rtl/objpas/classes/writer.inc	(revision 36212)
    +++ rtl/objpas/classes/writer.inc	(working copy)
    @@ -985,7 +985,7 @@
             if HasAncestor then
               DefStrValue := GetStrProp(Ancestor, PropInfo)
             else
    -          SetLength(DefStrValue, 0);
    +          DefStrValue := #0;
     
             if StrValue <> DefStrValue then
             begin
    @@ -1002,7 +1002,7 @@
             if HasAncestor then
               WDefStrValue := GetWideStrProp(Ancestor, PropInfo)
             else
    -          SetLength(WDefStrValue, 0);
    +          WDefStrValue := #0;
     
             if WStrValue <> WDefStrValue then
             begin
    @@ -1017,7 +1017,7 @@
             if HasAncestor then
               UDefStrValue := GetUnicodeStrProp(Ancestor, PropInfo)
             else
    -          SetLength(UDefStrValue, 0);
    +          UDefStrValue := #0;
     
             if UStrValue <> UDefStrValue then
             begin
    
  • EmptyStringWriter.lpr (1,374 bytes)
  • writer-empty-string-2.patch (3,991 bytes)
    Index: compiler/defutil.pas
    ===================================================================
    --- compiler/defutil.pas	(revision 36463)
    +++ compiler/defutil.pas	(working copy)
    @@ -205,6 +205,9 @@
         {# Returns true if p is a short string type }
         function is_shortstring(p : tdef) : boolean;
     
    +    {# Returns true if p is any pointer def }
    +    function is_pointer(p : tdef) : boolean;
    +
         {# Returns true if p is a pchar def }
         function is_pchar(p : tdef) : boolean;
     
    @@ -850,6 +853,12 @@
                                     is_widechar(tarraydef(p).elementdef);
           end;
     
    +    { true if p is any pointer def }
    +    function is_pointer(p : tdef) : boolean;
    +      begin
    +        is_pointer:=(p.typ=pointerdef);
    +      end;
    +
         { true if p is a pchar def }
         function is_pchar(p : tdef) : boolean;
           begin
    Index: compiler/pdecvar.pas
    ===================================================================
    --- compiler/pdecvar.pas	(revision 36463)
    +++ compiler/pdecvar.pas	(working copy)
    @@ -222,6 +222,15 @@
                  end;
               end;
     
    +          function has_implicit_default(p : tpropertysym) : boolean;
    +
    +          begin
    +             has_implicit_default:=
    +               (is_string(p.propdef) or
    +               is_real(p.propdef) or
    +               is_pointer(p.propdef));
    +          end;
    +
               function allow_default_property(p : tpropertysym) : boolean;
     
               begin
    @@ -652,6 +661,10 @@
                       end;
                   end;
                end;
    +         if has_implicit_default(p) then
    +           begin
    +              p.default:=0;
    +           end;
              if not is_record(astruct) and try_to_consume(_DEFAULT) then
                begin
                   if not allow_default_property(p) then
    Index: rtl/objpas/classes/writer.inc
    ===================================================================
    --- rtl/objpas/classes/writer.inc	(revision 36463)
    +++ rtl/objpas/classes/writer.inc	(working copy)
    @@ -944,7 +944,7 @@
               DefValue :=PPropInfo(PropInfo)^.Default;
               DefFloatValue:=PSingle(@PPropInfo(PropInfo)^.Default)^;
               end;
    -        if (FloatValue<>DefFloatValue) or (DefValue=longint($80000000)) then
    +        if (FloatValue<>DefFloatValue) or (not HasAncestor and (DefValue=longint($80000000))) then
             begin
               Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
               WriteFloat(FloatValue);
    @@ -985,9 +985,12 @@
             if HasAncestor then
               DefStrValue := GetStrProp(Ancestor, PropInfo)
             else
    +        begin
    +          DefValue :=PPropInfo(PropInfo)^.Default;
               SetLength(DefStrValue, 0);
    +        end;
     
    -        if StrValue <> DefStrValue then
    +        if (StrValue<>DefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
             begin
               Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
               if Assigned(FOnWriteStringProperty) then
    @@ -1002,9 +1005,12 @@
             if HasAncestor then
               WDefStrValue := GetWideStrProp(Ancestor, PropInfo)
             else
    +        begin
    +          DefValue :=PPropInfo(PropInfo)^.Default;
               SetLength(WDefStrValue, 0);
    +        end;
     
    -        if WStrValue <> WDefStrValue then
    +        if (WStrValue<>WDefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
             begin
               Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
               WriteWideString(WStrValue);
    @@ -1017,9 +1023,12 @@
             if HasAncestor then
               UDefStrValue := GetUnicodeStrProp(Ancestor, PropInfo)
             else
    +        begin
    +          DefValue :=PPropInfo(PropInfo)^.Default;
               SetLength(UDefStrValue, 0);
    +        end;
     
    -        if UStrValue <> UDefStrValue then
    +        if (UStrValue<>UDefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
             begin
               Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
               WriteUnicodeString(UStrValue);
    
  • test_1.lpr (2,941 bytes)

Activities

Ondrej Pokorny

2017-06-08 23:48

developer  

writer-empty-string-1.patch (975 bytes)
Index: rtl/objpas/classes/writer.inc
===================================================================
--- rtl/objpas/classes/writer.inc	(revision 36212)
+++ rtl/objpas/classes/writer.inc	(working copy)
@@ -985,7 +985,7 @@
         if HasAncestor then
           DefStrValue := GetStrProp(Ancestor, PropInfo)
         else
-          SetLength(DefStrValue, 0);
+          DefStrValue := #0;
 
         if StrValue <> DefStrValue then
         begin
@@ -1002,7 +1002,7 @@
         if HasAncestor then
           WDefStrValue := GetWideStrProp(Ancestor, PropInfo)
         else
-          SetLength(WDefStrValue, 0);
+          WDefStrValue := #0;
 
         if WStrValue <> WDefStrValue then
         begin
@@ -1017,7 +1017,7 @@
         if HasAncestor then
           UDefStrValue := GetUnicodeStrProp(Ancestor, PropInfo)
         else
-          SetLength(UDefStrValue, 0);
+          UDefStrValue := #0;
 
         if UStrValue <> UDefStrValue then
         begin

Thaddy de Koning

2017-06-09 09:04

reporter   ~0100959

I can confirm that this scenario indeed is a real issue on both Linux arm and Win64. It is also imo not a corner case.

Ondrej Pokorny

2017-06-09 09:08

developer  

EmptyStringWriter.lpr (1,374 bytes)

Ondrej Pokorny

2017-06-10 14:56

developer   ~0100993

Link to the docs: [url]https://www.freepascal.org/docs-html/ref/refsu38.html#x90-1120006.6.6[/url]

The docs are quite clear about (1), (2) and (3) - all should be streamed by the writer.

Serge Anvarov

2017-06-10 17:08

reporter   ~0100995

A very interesting topic. According to the documentation, for properties of type string you cannot use the default or nodefault. In the link above "The default specifier can be specified for ordinal types and sets". The same in Delphi docs: "The default and nodefault directives are supported only for ordinal types and for set types". Thus, the implementation depends on the compiler and is not mandatory.
For example, in Delphi 7, the same is done as in FPC - the empty string is not stored. But in Delphi 10.2 it is stored.

Ondrej Pokorny

2017-06-10 17:43

developer   ~0100998

Thanks for mentioning the Delphi docs.

Link to the Delphi docs: [url]http://docwiki.embarcadero.com/RADStudio/Tokyo/en/Properties_(Delphi)#Storage_Specifiers[/url].

FPC docs don't tell anything about the fact that nodefault isn't supported for string properties. Delphi docs do, on the other hand - nodefault shouldn't be supported for string properties.

Unfortunately, the current FPC behavior matches the Delphi documentation; it doesn't match the FPC documentation, though.

From Delphi docs: "If a property's current value is different from its default value (or if there is no default value) and the stored specifier is True, then the property's value is saved." :/

The problem is: this behavior effectively disallows streaming string properties with a non-empty default string :/

Delphi behavior:
Delphi 7: no strings are stored
Delphi 10.2: T is stored (S, U are not) => Delphi 10.2 supports the nodefault modifier for string properties, contrary to the docs.

Ondrej Pokorny

2017-06-10 17:54

developer   ~0100999

Note: the code below is the way to handle string properties with non-empty default value in Delphi 10.2.
1.) The default string ('default') isn't stored.
2.) The empty string ('') is stored.
3.) All other strings are stored as well.

=> Delphi 10.2 supports the nodefault modifier to remove the implicit empty-string default value of the property.
FPC ignores the nodefault modifier in this example.

Code:

type
  TMyComp = class(TComponent)
  private const
    cDefS = 'default';
  private
    fS: string;
    function SStored: Boolean;
  public
    constructor Create(AOwner: TComponent); override;
  published
    property S: string read fS write fS stored SStored nodefault;
  end;

{ TMyComp }

constructor TMyComp.Create(AOwner: TComponent);
begin
  inherited Create(AOwner);

  fS := cDefS;
end;

function TMyComp.SStored: Boolean;
begin
  Result := fS <> cDefS;
end;

Serge Anvarov

2017-06-10 18:32

reporter   ~0101001

In the context of the topic "Storage information", the rules for default should be propagated for nodefault, because It essentially denies the stored properties for default. So for him the statement "can be specified for ordinal types and sets" is also true.
So we can think of this as not a bug, but a feature request: implement support specifier default and nodefault for properties with a string type, and document this.

Ondrej Pokorny

2017-06-10 19:01

developer  

writer-empty-string-2.patch (3,991 bytes)
Index: compiler/defutil.pas
===================================================================
--- compiler/defutil.pas	(revision 36463)
+++ compiler/defutil.pas	(working copy)
@@ -205,6 +205,9 @@
     {# Returns true if p is a short string type }
     function is_shortstring(p : tdef) : boolean;
 
+    {# Returns true if p is any pointer def }
+    function is_pointer(p : tdef) : boolean;
+
     {# Returns true if p is a pchar def }
     function is_pchar(p : tdef) : boolean;
 
@@ -850,6 +853,12 @@
                                 is_widechar(tarraydef(p).elementdef);
       end;
 
+    { true if p is any pointer def }
+    function is_pointer(p : tdef) : boolean;
+      begin
+        is_pointer:=(p.typ=pointerdef);
+      end;
+
     { true if p is a pchar def }
     function is_pchar(p : tdef) : boolean;
       begin
Index: compiler/pdecvar.pas
===================================================================
--- compiler/pdecvar.pas	(revision 36463)
+++ compiler/pdecvar.pas	(working copy)
@@ -222,6 +222,15 @@
              end;
           end;
 
+          function has_implicit_default(p : tpropertysym) : boolean;
+
+          begin
+             has_implicit_default:=
+               (is_string(p.propdef) or
+               is_real(p.propdef) or
+               is_pointer(p.propdef));
+          end;
+
           function allow_default_property(p : tpropertysym) : boolean;
 
           begin
@@ -652,6 +661,10 @@
                   end;
               end;
            end;
+         if has_implicit_default(p) then
+           begin
+              p.default:=0;
+           end;
          if not is_record(astruct) and try_to_consume(_DEFAULT) then
            begin
               if not allow_default_property(p) then
Index: rtl/objpas/classes/writer.inc
===================================================================
--- rtl/objpas/classes/writer.inc	(revision 36463)
+++ rtl/objpas/classes/writer.inc	(working copy)
@@ -944,7 +944,7 @@
           DefValue :=PPropInfo(PropInfo)^.Default;
           DefFloatValue:=PSingle(@PPropInfo(PropInfo)^.Default)^;
           end;
-        if (FloatValue<>DefFloatValue) or (DefValue=longint($80000000)) then
+        if (FloatValue<>DefFloatValue) or (not HasAncestor and (DefValue=longint($80000000))) then
         begin
           Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
           WriteFloat(FloatValue);
@@ -985,9 +985,12 @@
         if HasAncestor then
           DefStrValue := GetStrProp(Ancestor, PropInfo)
         else
+        begin
+          DefValue :=PPropInfo(PropInfo)^.Default;
           SetLength(DefStrValue, 0);
+        end;
 
-        if StrValue <> DefStrValue then
+        if (StrValue<>DefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
         begin
           Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
           if Assigned(FOnWriteStringProperty) then
@@ -1002,9 +1005,12 @@
         if HasAncestor then
           WDefStrValue := GetWideStrProp(Ancestor, PropInfo)
         else
+        begin
+          DefValue :=PPropInfo(PropInfo)^.Default;
           SetLength(WDefStrValue, 0);
+        end;
 
-        if WStrValue <> WDefStrValue then
+        if (WStrValue<>WDefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
         begin
           Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
           WriteWideString(WStrValue);
@@ -1017,9 +1023,12 @@
         if HasAncestor then
           UDefStrValue := GetUnicodeStrProp(Ancestor, PropInfo)
         else
+        begin
+          DefValue :=PPropInfo(PropInfo)^.Default;
           SetLength(UDefStrValue, 0);
+        end;
 
-        if UStrValue <> UDefStrValue then
+        if (UStrValue<>UDefStrValue) or (not HasAncestor and (DefValue=longint($80000000))) then
         begin
           Driver.BeginProperty(FPropPath + PPropInfo(PropInfo)^.Name);
           WriteUnicodeString(UStrValue);

Ondrej Pokorny

2017-06-10 19:02

developer  

test_1.lpr (2,941 bytes)

Ondrej Pokorny

2017-06-10 19:07

developer   ~0101006

writer-empty-string-2.patch: I updated a new patch to match Delphi behavior.

It adds support for the nodefault modifier for string/float properties. To do so it fixes the PPropInfo(PropInfo)^.Default value for string/float properties without nodefault: new=0, old=$80000000. Delphi stores 0 in this case.

It also fixes the uninitialized DefValue for tkFloat properties.

I also created a test case for the test suite (test_1.lpr). I hope it can be applied.

Michael Van Canneyt

2018-01-13 11:23

administrator   ~0105735

Applied & tested patch, added test to testsuite. Thank you very much !

Ondrej Pokorny

2018-01-14 10:28

developer   ~0105780

Thank you!

Issue History

Date Modified Username Field Change
2017-06-08 23:48 Ondrej Pokorny New Issue
2017-06-08 23:48 Ondrej Pokorny File Added: writer-empty-string-1.patch
2017-06-09 09:04 Thaddy de Koning Note Added: 0100959
2017-06-09 09:08 Ondrej Pokorny File Added: EmptyStringWriter.lpr
2017-06-10 14:56 Ondrej Pokorny Note Added: 0100993
2017-06-10 17:08 Serge Anvarov Note Added: 0100995
2017-06-10 17:43 Ondrej Pokorny Note Added: 0100998
2017-06-10 17:54 Ondrej Pokorny Note Added: 0100999
2017-06-10 18:32 Serge Anvarov Note Added: 0101001
2017-06-10 19:01 Ondrej Pokorny File Added: writer-empty-string-2.patch
2017-06-10 19:02 Ondrej Pokorny File Added: test_1.lpr
2017-06-10 19:07 Ondrej Pokorny Note Added: 0101006
2017-07-09 10:08 Michael Van Canneyt Category RTL => Compiler
2018-01-12 07:54 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-01-12 07:54 Michael Van Canneyt Status new => assigned
2018-01-13 11:23 Michael Van Canneyt Fixed in Revision => 37954
2018-01-13 11:23 Michael Van Canneyt Note Added: 0105735
2018-01-13 11:23 Michael Van Canneyt Status assigned => resolved
2018-01-13 11:23 Michael Van Canneyt Fixed in Version => 3.1.1
2018-01-13 11:23 Michael Van Canneyt Resolution open => fixed
2018-01-13 11:23 Michael Van Canneyt Target Version => 3.2.0
2018-01-14 10:28 Ondrej Pokorny Note Added: 0105780
2018-01-14 10:28 Ondrej Pokorny Status resolved => closed