Before the main course, I offer the attached refactorings for trecorddef.create_global_internal:
1) streamline insertions into the symtable;
2) avoid shadow-copying of the parameter "n", which is now const

Am 03.08.2019 um 15:54 schrieb

Before the main course, I offer the attached refactorings for trecorddef.create_global_internal: 1) streamline insertions into the symtable;

Applied in r42652

2) avoid shadow-copying of the parameter "n", which is now const

Applied in r42653

Regards,
Sven

Very eager for closures. Btw, what’s the status Blaise? I didn’t hear back since last month or so.

Regards,
Ryan Joseph

trecorddef.create_global_internal

Currently, autogenerated internal names are composed as '$InternalRec'+tostr(current_module.deflist.count) IMO, this is misleading: it appears as if such names 1) would be unique within a module, and 2) would encode the respective DefIDs; in actuality, neither happens (since such internal RecordDefs are not necessarily registered).

This can be demonstrated by logging
writeln(current_module.in_interface, #9, objrealname^) @ create_global_internal.
For a simple unit
-------8<-------
unit U;
interface

	type C = class end;

implementation
end.
-------8<-------
it shows

So, what is the point?
Initially, I wanted to use deflist.count from the "where" symbol table (i.e. current_module.localsymtable or .globalsymtable; or a specified table -- for my upcoming Scoped VMTDefs patch) instead. But, wouldn't it be better to drop that misleading suffix altogether?

My understanding is that a name should never be autogenerated for an internal RecordDef that is going to be registered with a DefID, thus, we can even ASSERT( not current_module.in_interface ) in that case.

A draft patch is attached. The assertion comes in two flavours -- decide which (if any) is appropriate.

{ This is a resubmission of https://lists.freepascal.org/pipermail/fpc-devel/2019-August/041915.html }

trecorddef.create_global_internal generates internal RecordDef names as '$InternalRec'+tostr(current_module.deflist.count) However, since such internal RecordDefs are not necessarily registered afterwards (i.e. deflist.count is not going to be bumped), such names do not contain actual DefIDs, and thus are not unique within a module. (See the original email for a test case.)

1) So, what is the point of that misleading numerical suffix? I propose that it be dropped.
2) My understanding is that a name should never be autogenerated for an internal RecordDef that is going to be registered with a DefID. I propose that this be asserted (two variants are offered).

The same draft patch is attached.

Am 30.11.2019 um 13:27 schrieb

I've fixed this now in r43616 by using the correct way to generate a unique ID string. The LLVM backend makes use of anonymous record defs and I don't know whether your patch would break that.

Regards,
Sven

You are invoking unique_id_str before the inherited constructor sets defid:=defid_not_registered. Before that, defid is 0, so the aforementioned test case shows:

At the very least:
	defid:=defid_not_registered;
	name:='$InternalRec'+unique_id_str;
which I find hacky.
  The LLVM backend makes use of anonymous record defs and I don't know whether your patch would break that.

Presumably, if such duplicate internal names were breaking LLVM, it would have been noticed already? Maybe Jonas could weigh in?

Am 01.12.2019 um 11:02 schrieb

Fixed in r43624 and r43625. I've decided to keep the unique names, because they are useful when debugging. More useful then say the Self value.

Regards,
Sven