View Issue Details

IDProjectCategoryView StatusLast Update
0030534FPCCompilerpublic2017-04-05 15:48
ReporterMaciej Izak Assigned ToMaciej Izak  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.1.1 
Fixed in Version3.1.1 
Summary0030534: [patch] for Implicit/Explicit generic class operators
DescriptionAnother patch from hated SmartPointers effort ;). Very important part for TSmartObj.Implicit:

https://github.com/maciej-izak/PascalSmartPointers/blob/master/SmartObj.pas

Without patch FPC raise error: "Impossible to overload assignment for equal types."
Patch is also Delphi compatible.

Test and patch attached.
Tagsgenerics
Fixed in Revision34526, 35740
FPCOldBugId
FPCTarget
Attached Files

Activities

Maciej Izak

2016-08-31 10:52

reporter  

compiler_patch_implicit_explicit_31.08.2016.patch (3,845 bytes)   
Index: compiler/defcmp.pas
===================================================================
--- compiler/defcmp.pas	(revision 34382)
+++ compiler/defcmp.pas	(working copy)
@@ -59,7 +59,8 @@
           cdo_allow_variant,
           cdo_parameter,
           cdo_warn_incompatible_univ,
-          cdo_strict_undefined_check  // undefined defs are incompatible to everything except other undefined defs
+          cdo_strict_undefined_check, // undefined defs are incompatible to everything except other undefined defs
+          cdo_ignore_genconstraintdata // used for class operators Implicit and Explicit (is required to not break test tgenfunc12.pp)
        );
        tcompare_defs_options = set of tcompare_defs_option;
 
@@ -118,6 +119,11 @@
     { Returns true, if def1 and def2 are semantically the same }
     function equal_defs(def_from,def_to:tdef):boolean;
 
+    { Returns true, if def1 and def2 are semantically the same,
+      different than standard equal_defs, required for generic
+      operators implicit and explicit }
+    function equal_defs_assignment_op(def_from,def_to:tdef):boolean;
+
     { Checks for type compatibility (subgroups of type)
       used for case statements... probably missing stuff
       to use on other types }
@@ -276,13 +282,19 @@
          else
            begin
              { undefined defs or defs with generic constraints are
-               considered equal to everything }
+               considered equal to everything (except generic assignment operators) }
              if (
                    (def_from.typ=undefineddef) or
-                   assigned(tstoreddef(def_from).genconstraintdata)
+                     (
+                       assigned(tstoreddef(def_from).genconstraintdata) and
+                       not (cdo_ignore_genconstraintdata in cdoptions)
+                     )
                  ) or (
                    (def_to.typ=undefineddef) or
-                   assigned(tstoreddef(def_to).genconstraintdata)
+                     (
+                       assigned(tstoreddef(def_to).genconstraintdata) and
+                       not (cdo_ignore_genconstraintdata in cdoptions)
+                     )
                  ) then
               begin
                 doconv:=tc_equal;
@@ -1872,6 +1884,16 @@
         equal_defs:=(compare_defs_ext(def_from,def_to,nothingn,convtyp,pd,[])>=te_equal);
       end;
 
+    function equal_defs_assignment_op(def_from,def_to:tdef):boolean;
+      var
+        convtyp : tconverttype;
+        pd : tprocdef;
+      begin
+        { Compare defs with nothingn and no explicit typecasts and
+          searching for overloaded operators is not needed, additionaly
+          put special flag for generic assignment operators implicit and explicit }
+        equal_defs_assignment_op:=(compare_defs_ext(def_from,def_to,nothingn,convtyp,pd,[cdo_ignore_genconstraintdata])>=te_equal);
+      end;
 
     function compare_defs(def_from,def_to:tdef;fromtreetype:tnodetype):tequaltype;
       var
Index: compiler/pdecsub.pas
===================================================================
--- compiler/pdecsub.pas	(revision 34382)
+++ compiler/pdecsub.pas	(working copy)
@@ -1431,7 +1431,7 @@
                          MessagePos(pd.fileinfo,type_e_type_id_expected);
                    end;
                  if (optoken in [_ASSIGNMENT,_OP_EXPLICIT]) and
-                    equal_defs(pd.returndef,tparavarsym(pd.parast.SymList[0]).vardef) and
+                    equal_defs_assignment_op(pd.returndef,tparavarsym(pd.parast.SymList[0]).vardef) and
                     (pd.returndef.typ<>undefineddef) and (tparavarsym(pd.parast.SymList[0]).vardef.typ<>undefineddef) then
                    message(parser_e_no_such_assignment)
                  else if not isoperatoracceptable(pd,optoken) then

Maciej Izak

2016-08-31 10:52

reporter  

test.pp (257 bytes)

Maciej Izak

2016-08-31 10:56

reporter   ~0094357

I forgot about { %NORUN } for test.

Thaddy de Koning

2016-09-01 11:29

reporter   ~0094363

Ooooh, I like that and I need that... I will test it for sure.

Thaddy de Koning

2016-09-01 11:35

reporter   ~0094364

You can also use the smartpointers example in Marco Cantu's (temporary) free Object Pascal book adapted for Delphi 10.1 to see what I mean (and probably Maciej means)

Thaddy de Koning

2016-09-03 09:40

reporter   ~0094384

Last edited: 2016-09-03 09:57

View 4 revisions

By now I have tested the patch with the above example from the book and it works as advertised. I also checked that it does not break default implicit.

The patch seems OK. All code works. I tested on armhf-linux with 34415 + patch.

I am not sure if I am allowed to attach the example code, but I have it available when needed.
[edit]
Sourcecodeis on github https://github.com/MarcoDelphiBooks/ObjectPascalHandbook
An example is in the chapter 14 directory.
I used the code from the book, though, to create a console app.

Sven Barth

2016-09-16 15:26

manager   ~0094679

Thank you for your patch, but I haven't used it, 'cause that would only have hidden the problem, namely that it's incorrect to consider everything as compatible with a type that has generic constraints.
The new code is still not perfect, but for now (and your use case) it's sufficient.

Maciej Izak

2016-10-07 23:12

reporter   ~0095011

Sven, the path in 34526 won't work with "record" constraint (which is badly implemented in FPC see 0024073 - btw. patch attached):

TSmartPtr<T: record> = record

See new test2.pp. nullable/nilable types implementation in NewPascal branch is broken. With my patch in this report all works good.

Maciej Izak

2016-10-07 23:12

reporter  

test2.pp (303 bytes)

Maciej Izak

2016-10-08 00:30

reporter  

0001-Fix-for-assignment-operators-with-constraint-like-re.patch (3,638 bytes)   
From 53b26436961140d85a278872349963d665d686c6 Mon Sep 17 00:00:00 2001
From: maciej-izak <hnb.code@gmail.com>
Date: Sat, 8 Oct 2016 00:28:32 +0200
Subject: [PATCH] Fix for assignment operators with constraint like record. We
 need for that case to know the context. Patch for
 http://bugs.freepascal.org/view.php?id=30534.

---
 compiler/defcmp.pas  | 23 +++++++++++++++++++++--
 compiler/pdecsub.pas |  2 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/compiler/defcmp.pas b/compiler/defcmp.pas
index 3d92083..a33a079 100644
--- a/compiler/defcmp.pas
+++ b/compiler/defcmp.pas
@@ -59,7 +59,8 @@ interface
           cdo_allow_variant,
           cdo_parameter,
           cdo_warn_incompatible_univ,
-          cdo_strict_undefined_check  // undefined defs are incompatible to everything except other undefined defs
+          cdo_strict_undefined_check,  // undefined defs are incompatible to everything except other undefined defs
+          cdo_assign_operator_dec
        );
        tcompare_defs_options = set of tcompare_defs_option;
 
@@ -115,6 +116,11 @@ interface
     { Returns if the type def_from can be converted to def_to or if both types are equal }
     function compare_defs(def_from,def_to:tdef;fromtreetype:tnodetype):tequaltype;
 
+    { Returns true, if def1 and def2 are semantically the same,
+      different than standard equal_defs, required for generic
+      operators implicit and explicit }
+    function equal_defs_assignment_op_dec(def_from,def_to:tdef):boolean;
+
     { Returns true, if def1 and def2 are semantically the same }
     function equal_defs(def_from,def_to:tdef):boolean;
 
@@ -288,7 +294,8 @@ implementation
              if assigned(tstoreddef(def_from).genconstraintdata) or
                  assigned(tstoreddef(def_to).genconstraintdata) then
                begin
-                 if def_from.typ<>def_to.typ then
+                 if (cdo_assign_operator_dec in cdoptions) or
+                     (def_from.typ=def_to.typ) then
                    begin
                      { not compatible anyway }
                      doconv:=tc_not_possible;
@@ -1875,6 +1882,18 @@ implementation
       end;
 
 
+    function equal_defs_assignment_op_dec(def_from,def_to:tdef):boolean;
+      var
+        convtyp : tconverttype;
+        pd : tprocdef;
+      begin
+        { Compare defs with nothingn and no explicit typecasts and
+          searching for overloaded operators is not needed, additionaly
+          put special flag for generic assignment operators implicit and explicit }
+        equal_defs_assignment_op_dec:=(compare_defs_ext(def_from,def_to,nothingn,convtyp,pd,[cdo_assign_operator_dec])>=te_equal);
+      end;
+
+
     function equal_defs(def_from,def_to:tdef):boolean;
       var
         convtyp : tconverttype;
diff --git a/compiler/pdecsub.pas b/compiler/pdecsub.pas
index e354cd6..a15aef4 100644
--- a/compiler/pdecsub.pas
+++ b/compiler/pdecsub.pas
@@ -1482,7 +1482,7 @@ implementation
                            MessagePos(pd.fileinfo,type_e_type_id_expected);
                      end;
                    if (optoken in [_ASSIGNMENT,_OP_EXPLICIT]) and
-                      equal_defs(pd.returndef,tparavarsym(pd.parast.SymList[0]).vardef) and
+                      equal_defs_assignment_op_dec(pd.returndef,tparavarsym(pd.parast.SymList[0]).vardef) and
                       (pd.returndef.typ<>undefineddef) and (tparavarsym(pd.parast.SymList[0]).vardef.typ<>undefineddef) then
                      message(parser_e_no_such_assignment)
                    else if not isoperatoracceptable(pd,optoken) then
-- 
2.9.3.windows.2

Maciej Izak

2016-10-08 00:30

reporter   ~0095012

Sven, to fix this for assignment operators we need to know context in compare_defs_ext. Improved patch attached.

Maciej Izak

2016-10-08 01:41

reporter   ~0095013

Patch "0001-Fix-for-assignment-operators-with-constraint-like-re.patch" contains small typo ("=" instead of "<>")from my test :\. Correction for previous patch attached.

Maciej Izak

2016-10-08 01:41

reporter  

0001-Small-typo-usage-instead-of-.-Regression-detected-an.patch (892 bytes)   
From f890ce9436f5714e3b765f59b542ee1ba85f635b Mon Sep 17 00:00:00 2001
From: maciej-izak <hnb.code@gmail.com>
Date: Sat, 8 Oct 2016 01:39:24 +0200
Subject: [PATCH] Small typo, usage "=" instead of "<>". Regression detected
 and fixed!

---
 compiler/defcmp.pas | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler/defcmp.pas b/compiler/defcmp.pas
index a33a079..0b2203d 100644
--- a/compiler/defcmp.pas
+++ b/compiler/defcmp.pas
@@ -295,7 +295,7 @@ implementation
                  assigned(tstoreddef(def_to).genconstraintdata) then
                begin
                  if (cdo_assign_operator_dec in cdoptions) or
-                     (def_from.typ=def_to.typ) then
+                     (def_from.typ<>def_to.typ) then
                    begin
                      { not compatible anyway }
                      doconv:=tc_not_possible;
-- 
2.9.3.windows.2

Maciej Izak

2016-10-08 23:57

reporter   ~0095033

Additionally r34526 has regression for below code, very visible with patch 0024073 (I can provide patch for this scenario too but 0024073 is not accepted yet) :

type
  TNullable<T: record> = record
  public type
    PT = ^T;
  strict private
    Instance: ^T ;
  public
    class operator Equal(A: TNullable<T>; B: PT): Boolean;
    class operator Equal(A: TNullable<T>; B: T): Boolean;
  end;

...

var
  x: TNullable<Integer>;

Issue History

Date Modified Username Field Change
2016-08-31 10:52 Maciej Izak New Issue
2016-08-31 10:52 Maciej Izak File Added: compiler_patch_implicit_explicit_31.08.2016.patch
2016-08-31 10:52 Maciej Izak File Added: test.pp
2016-08-31 10:54 Maciej Izak Tag Attached: generic
2016-08-31 10:54 Maciej Izak Tag Detached: generic
2016-08-31 10:55 Maciej Izak Tag Attached: generics
2016-08-31 10:56 Maciej Izak Note Added: 0094357
2016-09-01 11:29 Thaddy de Koning Note Added: 0094363
2016-09-01 11:35 Thaddy de Koning Note Added: 0094364
2016-09-03 09:40 Thaddy de Koning Note Added: 0094384
2016-09-03 09:42 Thaddy de Koning Note Edited: 0094384 View Revisions
2016-09-03 09:55 Thaddy de Koning Note Edited: 0094384 View Revisions
2016-09-03 09:57 Thaddy de Koning Note Edited: 0094384 View Revisions
2016-09-16 15:26 Sven Barth Fixed in Revision => 34526
2016-09-16 15:26 Sven Barth Note Added: 0094679
2016-09-16 15:26 Sven Barth Status new => resolved
2016-09-16 15:26 Sven Barth Fixed in Version => 3.1.1
2016-09-16 15:26 Sven Barth Resolution open => fixed
2016-09-16 15:26 Sven Barth Assigned To => Sven Barth
2016-10-07 23:12 Maciej Izak Note Added: 0095011
2016-10-07 23:12 Maciej Izak Status resolved => feedback
2016-10-07 23:12 Maciej Izak Resolution fixed => reopened
2016-10-07 23:12 Maciej Izak File Added: test2.pp
2016-10-08 00:30 Maciej Izak File Added: 0001-Fix-for-assignment-operators-with-constraint-like-re.patch
2016-10-08 00:30 Maciej Izak Note Added: 0095012
2016-10-08 00:30 Maciej Izak Status feedback => assigned
2016-10-08 01:41 Maciej Izak Note Added: 0095013
2016-10-08 01:41 Maciej Izak File Added: 0001-Small-typo-usage-instead-of-.-Regression-detected-an.patch
2016-10-08 23:57 Maciej Izak Note Added: 0095033
2017-04-05 14:57 Maciej Izak Assigned To Sven Barth => Maciej Izak
2017-04-05 15:48 Maciej Izak Fixed in Revision 34526 => 34526, 35740
2017-04-05 15:48 Maciej Izak Status assigned => resolved
2017-04-05 15:48 Maciej Izak Resolution reopened => fixed