Sign in Join

Live chat for Free Pascal, Lazarus, Delphi and every Pascal programmer.

Mailing lists / fpc-devel / 2020-04

Refactoring tobjectdef.vmt_def

5 messages · 3 people · –

Reply by email Subscribe to this list Your subscription

Read-only archive. Use j and k to move between messages.

Unknown author

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

βþ

Reply by email

Jonas Maebe
to Unknown author
Show quoted text 7 lines
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

Reply by email

Jonas Maebe
to Jonas Maebe
Jonas Maebe wrote 6 lines
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).

However, both toriginalclass tobjectdef and typesym are in the same symtable, so it should still be okay, actually. I would have revert the change and test/debug it again to find out why I thought this change was necessary.

I guess you want to revert it because you want to create tobjectdefs without a typesym?

Jonas

Reply by email

Jonas Maebe
to Jonas Maebe: However, both toriginalclass tobjectdef and typesym are in the same symtable, so it…
Jonas Maebe wrote 11 lines
On 21/04/2020 22:56, Jonas Maebe wrote: > 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). However, both toriginalclass tobjectdef and typesym are in the same symtable, so it should still be okay, actually. I would have revert the change and test/debug it again to find out why I thought this change was necessary.

I've checked and tested, and it's indeed safe to remove the dependency on the typesym, so I have removed it.

Jonas

Reply by email