On 21/04/2020 10:57, bla...@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 _______________________________________________ fpc-devel maillist - fpc-devel@lists.freepascal.org https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
