Ticket #631 (closed RFC: fixed)

Opened 5 years ago

Last modified 5 years ago

[RFC] Clean-up PMC generated files.

Reported by: bacek Owned by:
Priority: normal Milestone:
Component: none Version: 1.1.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

Hello.

Working on PMC parsing in pmc_pct branch I found few glitches in files generated from PMCs.

1. All methods decorated with PARROT_EXPORT. But they should not. Because only one way to invoke them is C<vtable> pointer in PMC.

2. All _ro_ variants of generated functions can be replaced by single implementation in C<default.pmc> similar to C<cant_do_method>

My proposal:

1. Drop PARROT_EXPORT from generated functions.

2. Stop putting signature of functions in generated header file (except for default.pmc)

3. Replace generating of readonly variants with reference to C<default.pmc>

--

Bacek

Change History

  Changed 5 years ago by whiteknight

I had thought about some of these things before, and I'm very glad that you mention them specifically. I very much like #3, and agree with it wholeheartedly. However, I think #1 and #2 are problematic. Consider the case of an externally-compiled dynpmc that extends a built-in pmc. src/dynpmc/foo.pmc is an example of this. In foo's vtable are a number of pointers to Parrot_Integer_* functions that are inherited from the Integer PMC vtable. These pointers can't be filled it (at least not the way things are now) if we drop PARROT_EXPORT from them and stop putting their signatures in the generated header files. Notice also that even for PMCs that do not inherit from a built-in PMC explicitly, all undefined vtables fall back to the implemntations in src/pmc/default.pmc, which then needs to be exported.

Now, that said, I think these are good goals to move towards. We *should* be able to stop exporting all those functions, and allow a smarter dynloader mechanism to fill in the vtable overrides automatically. Of course, this pushes the generation of vtables from build-time to run-time, which I'm sure some people won't like. There are probably a few smarter ways to do this, and we should all think about it.

  Changed 5 years ago by chromatic

TT #528 addresses the PARROT_EXPORT on vtable function problem.

  Changed 5 years ago by fperrad

Check your modification against dynPMC in languages Lua & WMLScript.

There are good example of inheritance. The inheritance of PMC Default is common case, but not the general case.

  Changed 5 years ago by cotto

VTABLE functions are no longer declared PARROT_EXPORT as of r38471. The other two goals (no VTABLE functions in pmc headers and using default ro VTABLE functions) have not been implemented.

follow-up: ↓ 6   Changed 5 years ago by Infinoid

Looks like the tt631_part2 branch was merged back into trunk by bacek in r38536. This causes warnings for me like the following:

./src/pmc/managedstruct.c: In function 'Parrot_ManagedStruct_get_vtable':
./src/pmc/managedstruct.c:1089: warning: implicit declaration of function 'Parrot_UnManagedStruct_update_vtable'
./src/pmc/managedstruct.c:1089: warning: nested extern declaration of 'Parrot_UnManagedStruct_update_vtable'
./src/pmc/managedstruct.c: In function 'Parrot_ManagedStruct_ro_get_vtable':
./src/pmc/managedstruct.c:1100: warning: implicit declaration of function 'Parrot_UnManagedStruct_ro_update_vtable'
./src/pmc/managedstruct.c:1100: warning: nested extern declaration of 'Parrot_UnManagedStruct_ro_update_vtable'

When building under g++, these warnings become errors (NotFound++ for testing this):

[07:34] <@NotFound> ./src/pmc/managedstruct.c: In function 'VTABLE* Parrot_ManagedStruct_get_vtable(parrot_interp_t*)':
[07:34] <@NotFound> ./src/pmc/managedstruct.c:1089: error: 'Parrot_UnManagedStruct_update_vtable' was not declared in this scope

Apparently the generated pmc_unmanagedstruct.h is missing some function prototypes which the subclasses need?

in reply to: ↑ 5   Changed 5 years ago by Infinoid

Replying to Infinoid:

Apparently the generated pmc_unmanagedstruct.h is missing some function prototypes which the subclasses need?

The prototypes were generated for the Default PMC, but apparently they are needed elsewhere, so apparently the Default PMC isn't a special case. I've checked in a partial fix for this as r38537. This seems to get the standard PMCs building without warning.

There was an additional issue with dynpmcs, fixed in r38538:

digest_group.c:75: warning: implicit declaration of function 'Parrot_MD2_class_init'
digest_group.c:76: warning: implicit declaration of function 'Parrot_RIPEMD160_class_init'

(and so on for all the other digest dynpmcs).

I'm not sure either of these commits are the correct way to fix the problems, but they make the warnings go away.

  Changed 5 years ago by NotFound

I've seen that the get_vtable functions call the update_vtable of the parent, and the grandparent... It will be a cleaner schema if the parent update called his parent update, and so on. That way, maybe we can even avoid the #include "pmc_grandparent.h". I know this is generated code, but even in that case reducing dependencies may be good.

  Changed 5 years ago by bacek

  • status changed from new to closed
  • resolution set to fixed

Third goal was addressed in tt631_part3 branch which merged at r38603. All issues where resolved, closing ticket.

Note: See TracTickets for help on using tickets.