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

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

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

Am 02.08.2019 um 21:27 schrieb

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

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

Am 02.08.2019 um 21:27 schrieb

I raised this on core and it was rejected. Maintainability is more important than performance as it could easily be that something else is added to insert_struct_hidden_paras and then that is forgotten to be added in TVMTBuilder.generate_vmt.

That proposal is accepted. However without an overload as I suggested, instead all callsites of handle_calling_convention will need to be adjusted. Feel free to draw up a patch for this.

Regards,
Sven