[fpc-devel] Refactoring tobjectdef.vmt_def

[Blaise at blaise.ru]

I have questions/suggestions WRT Jonas' SVN r42342:
https://github.com/graemeg/freepascal/commit/40d31e34116efffa239b6dc94373fcfccedfa646

1) Why was `typesym.owner` used instead of `owner`? Beside efficiency, this hurts understandability: is it possible for a typedef to reside in a symtable separate from typesym?

2) Why exactly was `get_top_level_symtable` replaced with `get_unit_symtable`? Despite different implementations, they appear to be functionally equivalent. Conceptually, the replacement does make sense; but the commit claims to "fix tobjectdef.vmt_def to search in the correct symtable", yet does not mention a bug report / test case.

3) Streamline the logic: invoke `Find` only once.

4) Drop `tdef.get_top_level_symtable`. It was only used by tobjectdef.vmt_def before r42342.

Attached are the patches "Amend" (for 1-3) and "Drop" (for 4).
Two currently commented out ICE checks in "Amend" need to be dropped. I had them tested against the FPC/RTL/test-suite/LCL/LazIDE, and those two were not triggered. Could (2) be specific to the LLVM back-end?
(Also, I have dropped the comment about the tabstractrecordsymtable typecast; I reckon it is rather obvious, especially now, having it reside right under the '[ObjectSymtable,recordsymtable]' condition.)

-- 
βþ
-------------- next part --------------
# HG changeset patch
# User Blaise.ru
TODO: WTF r42342

diff -r 0b5141d52cf0 -r b7add2e1e427 symdef.pas
--- a/symdef.pas
+++ b/symdef.pas
@@ -7815,14 +7815,15 @@
 
     function tobjectdef.vmt_def: trecorddef;
       var
+        where: tsymtable;
         vmttypesym: tsymentry;
       begin
-        if not(typesym.owner.symtabletype in [ObjectSymtable,recordsymtable]) then
-          vmttypesym:=typesym.owner.Find('vmtdef$'+mangledparaname)
-        else
-          { Use common parent of trecordsymtable and tobjectsymtable
-            to avoid invalid typecast error when compiled with -CR option }
-          vmttypesym:=tabstractrecordsymtable(typesym.owner).get_unit_symtable.Find('vmtdef$'+mangledparaname);
+        where:=owner;
+        //IF WHERE <> typesym.owner THEN INTERNALERROR(2020);
+        if where.symtabletype in [ObjectSymtable,recordsymtable] then
+          where:=tabstractrecordsymtable(where).get_unit_symtable;
+        //IF WHERE <> get_top_level_symtable THEN INTERNALERROR(2022);
+        vmttypesym:=where.Find('vmtdef$'+mangledparaname);
         if not assigned(vmttypesym) or
            (vmttypesym.typ<>symconst.typesym) or
            (ttypesym(vmttypesym).typedef.typ<>recorddef) then
-------------- next part --------------
# HG changeset patch
# User Blaise.ru
DROP tdef.get_top_level_symtable

diff -r b7add2e1e427 -r 16be4eecb14e symtype.pas
--- a/symtype.pas
+++ b/symtype.pas
@@ -97,7 +97,6 @@
          procedure ChangeOwner(st:TSymtable);
          function getreusablesymtab: tsymtable;
          procedure register_created_object_type;virtual;
-         function  get_top_level_symtable: tsymtable;
          { only valid for registered defs and defs for which a unique id string
            has been requested; otherwise, first call register_def }
          function  deflist_index: longint;
@@ -442,15 +441,6 @@
       end;
 
 
-    function tdef.get_top_level_symtable: tsymtable;
-      begin
-        result:=owner;
-        while assigned(result) and
-              assigned(result.defowner) do
-          result:=tdef(result.defowner).owner;
-      end;
-
-
     function tdef.deflist_index: longint;
       begin
         if defid<defid_not_registered then