Re: [fpc-devel] Streamlining TVMTBuilder.generate_vmt after r41716 & r41884
On 03.08.2019 15:01, Sven Barth via fpc-devel wrote:
In principle one could do that, though more often than not inside the compiler
maintainability beats performance. I'd prefer an opinion of Florian and/or
Jonas on this though...
Leaving the issue of current_filepos for a moment, the change would be this:
---8<---
{ VMT entry }
if is_new_vmt_entry(tprocdef(def),overridesclasshelper) then
add_new_vmt_entry(tprocdef(def),overridesclasshelper);
+{ hidden params }
+
handle_calling_convention(tprocdef(def),[hcc_insert_hidden_paras]);
end;
end;
-insert_struct_hidden_paras(_class);
build_interface_mappings;
---8<---
I would say, this is quite maintainable: replacing one call for another.
After r41884, the insertion of hidden parameters is already tightly coupled
with VMT generation. In fact, it is no longer VMTBuilder, it is now
ObjectDefPostprocessor :)
What difference would it make for closures? In the end you'd still need to
ensure that handle_calling_convention isn't called twice.
1) I would rather ensure it on a per-method basis;
2) I would rather add a check inside the combined loop above, instead of
modifying insert_struct_hidden_paras and needlessly affecting RECORDs.
--
βþ
___
fpc-devel maillist - fpc-devel@lists.freepascal.org
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
[fpc-devel] Scoped VMTDefs
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
--
βþ
# HG changeset patch
# User Blaise
# Date 1564833600 -10800
# Sat Aug 03 15:00:00 2019 +0300
# Node ID 2873de0e4992db706b03f52c007f6ef7b39f5f8a
# Parent b7e97cefc21b2ffdfb22c37abb9bbae8f8a8e7cc
= trecorddef.create_global_internal: streamline insertions into the symtable
diff -r b7e97cefc21b -r 2873de0e4992 symdef.pas
--- a/symdef.pasFri Aug 02 19:35:25 2019 +0300
+++ b/symdef.pasSat Aug 03 15:00:00 2019 +0300
@@ -4786,6 +4786,7 @@
constructor trecorddef.create_global_internal(n: string; packrecords,
recordalignmin: shortint);
var
oldsymtablestack: tsymtablestack;
+where: tsymtable;
ts: ttypesym;
definedname: boolean;
begin
@@ -4802,27 +4803,19 @@
symtable.defowner:=self;
isunion:=false;
inherited create(n,recorddef,true);
+where:=current_module.localsymtable;
+if not assigned(where) then
+ where:=current_module.globalsymtable;
+where.insertdef(self);
{ if we specified a name, then we'll probably want to look up the
type again by name too -> create typesym }
-ts:=nil;
if definedname then
begin
ts:=ctypesym.create(n,self,true);
{ avoid hints about unused types (these may only be used for
typed constant data) }
ts.increfcount;
- end;
-if assigned(current_module.localsymtable) then
- begin
-current_module.localsymtable.insertdef(self);
-if definedname then
- current_module.localsymtable.insert(ts);
- end
-else
- begin
-current_module.globalsymtable.insertdef(self);
-if definedname then
- current_module.globalsymtable.insert(ts);
+where.insert(ts);
end;
symtablestack:=oldsymtablestack;
{ don't create RTTI for internal types, these are not exported }
# HG changeset patch
# User Blaise
# Date 1564838933 -10800
# Sat Aug 03 16:28:53 2019 +0300
# Node ID 49cbbb22f46dc471223290b1b24f312f24d65e76
# Parent 2873de0e4992db706b03f52c007f6ef7b39f5f8a
= trecorddef.create_global_internal: avoid shadow-copying of the parameter "n",
which is now const
diff -r 2873de0e4992 -r 49cbbb22f46d symdef.pas
--- a/symdef.pasSat Aug 03 15:00:00 2019 +0300
+++ b/symdef.pasSat Aug 03 16:28:53 2019 +0300
@@ -374,7 +374,7 @@
variantrecdesc : pvariantrecdesc;
isunion : boolean;
constructor create(const n:string; p:TSymtable);virtual;
- constructor create_global_internal(n: string; packrecords,
recordalignmin: shortint); virtual;
+ constructor create_global_internal(const n: string; packrecords,
recordalignmin: shortint); virtual;
function add_field_by_def(const optionalname: TIDString; def: tdef):
tsym;
procedure add_fields_from_deflist(fieldtypes: tfplist);
constructor ppuload(ppufile:tcompilerppufile);
@@ -4783,33 +4783,37 @@
end;
-constructor trecorddef.create_global_internal(n: string; packrecords,
recordalignmin: shortint);
+constructor trecorddef.create_global_internal(const n: string;
packrecords, recordalignmin: shortint);
var
+name: string; pname: pshortstring;
oldsymtablestack: tsymtablestack;
where: tsymtable;
ts: ttypesym;
-definedname: boolean;
begin
{ construct name }
-definedname:=n<>'';
-if not definedname then
- n:='$InternalRec'+tostr(current_module.deflist.count);
+if n<>'' then
+ pname := @n
+else
+ begin
+name:='$InternalRec'+tostr(current_module.deflist.count);
+pname:=@name;
+ end;
oldsymtablestack:=symtablestack;
{ do not simply push/pop current_module.localsymtable, because
that can have side-effects (e.g., it removes helpers) }
symtablestack:=nil;
-symtable:=trecordsymtable.create(n,packrecords,recordalignmin);
+symtable:=trecordsymtable.create(pname^,packrecords,recordalignmin);
symtable.defowner:=self;
isunion:=false;
-inherited create(n,recorddef,true);
+inherited create(pname^,recorddef,true);//!
where:=current_module.localsymtable;
if not assigned(where) then
where:=current_module.globalsymtable;
where.insertdef(self);
{ if we specified a name, then we'll probably want to look up the
type again by name too -> create typesym }
-if definedname then
+if n<>'' then
begin
ts:=ctypesym.create(n,self,true);
{ avoid
Re: [fpc-devel] Streamlining TVMTBuilder.generate_vmt after r41716 & r41884
Am 02.08.2019 um 21:27 schrieb bla...@blaise.ru: On 02.08.2019 21:36, bla...@blaise.ru wrote: embed a copy of the body of insert_struct_hidden_paras into TVMTBuilder.generate_vmt, then merge those two procdef-member traversals into one (hey, performance!) Would you guys oppose such a change? Then we could rename insert_struct_hidden_paras back to insert_record_hidden_paras :) In principle one could do that, though more often than not inside the compiler maintainability beats performance. I'd prefer an opinion of Florian and/or Jonas on this though... Aside from performance, I would like it for closures (for their nameless methods, the insertion of hidden parameters cannot be deferred until the VMT generation). What difference would it make for closures? In the end you'd still need to ensure that handle_calling_convention isn't called twice. Also, handle_calling_convention would need to be changed not to indirectly rely on current_filepos, but I see that as a bonus: the trick of swapping current_filepos could be removed from its callers (namely, insert_record_hidden_paras). That would mean that the functions called inside handle_calling_convention (mainly those inside the if-clause for hcc_insert_hidden_paras) would have to be adjusted to handle that as well (especially lovely for those invoked by ForEachCall). If this is done there should also be an overload of handle_calling_convention that does not take a tfileposinfo argument and instead passes on current_filepos so that those code parts that just want to use the current position don't need to be changed. Regards, Sven ___ fpc-devel maillist - fpc-devel@lists.freepascal.org https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
Re: [fpc-devel] [Suggestion] Enumeration range-check intrinsic
On Sat, 3 Aug 2019, Ondrej Pokorny wrote: On 13.07.2019 21:26, Michael Van Canneyt wrote: I think all sides have now been reviewed to the point of boring or annoying each other almost to death, and we finally need to decide on whether the patch is applied, and if so, which parts of it. That would be great. I have been waiting for some kind of decision or at least some statement from the compiler team for more than a year. Are there any news regarding the decision progress? It has not been discussed on core. As far as I am concerned, the patch can be applied. But since I don't manage the compiler, I have little voice in the process. I will raise the subject on core. Michael. ___ fpc-devel maillist - fpc-devel@lists.freepascal.org https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
Re: [fpc-devel] [Suggestion] Enumeration range-check intrinsic
On 13.07.2019 21:26, Michael Van Canneyt wrote: I think all sides have now been reviewed to the point of boring or annoying each other almost to death, and we finally need to decide on whether the patch is applied, and if so, which parts of it. That would be great. I have been waiting for some kind of decision or at least some statement from the compiler team for more than a year. Are there any news regarding the decision progress? Ondrej ___ fpc-devel maillist - fpc-devel@lists.freepascal.org https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
