[fpc-devel] Refactoring tobjectdef.vmt_def

[Jonas Maebe]

On 21/04/2020 10:57, Blaise at blaise.ru wrote:
> 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?

They can be in a different symtable in case of a non-unique type alias.
I.e., "type tclassalias = toriginalclass", where "toriginalclass" was
declared in the interface and "tclassalias" is in the implementation, or
"toriginalclass" was declared in another unit than "tclassalias", or at
a higher level of a nested class type hierarchy.

What happens in that case, is

tclassalias ttypesym contains a reference to toriginalclass' tobjectdef,
and that tobjectdef's typesym be to toriginalclass' typesym. And in the
unit symtable holding that typesym, we will find the vmtdef we need
(because it got created when toriginalclass was defined).

> 2) Why exactly was `get_top_level_symtable` replaced with
> `get_unit_symtable`? Despite different implementations, they appear to
> be functionally equivalent.

They are not equivalent:
1) get_top_level_symtable starts searching from a def, and hence cannot
be used to start searching from a sym/sym's owner (unless you start
searching from sym.owner.defowner, but that one may not exist in case
the owner is already a static/globalsymtable -- then again, we already
check for that here as well)
2) while not a difference in this case, get_top_level_symtable will in
fact always return the top-most symtable (a static or global symtable of
the declaring unit/program). get_unit_symtable OTOH seems like a
misnomer, because it will stop when you encounter a
non-record/objectdef, such as a procdef. So for types defined locally in
a procedure/function, it will return that routine's localsymtable.

As mentioned, point 2) cannot not cause a difference in this case
because you cannot define classes/objects locally. Still,
get_unit_symtable should probably either be moved from
tabstractrecordsymtable to tobjectsymtable, be renamed (not sure to what
to though), or be changed to use the logic of get_top_level_symtable (if
that is compatible with all existing uses of get_unit_symtable).

> 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.

It was probably also during the compilation of sdo_dataobject using the
LLVM code generator.

> 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? 

Yes, because the other code generators do not need to look up the
vmt_def anymore later on. It's also why the vmt_def only needs to get
registered for inclusion in the ppu when using the LLVM codegenerator.


Jonas