review of context_pmc3 branch

[Vasily Chekalkin]

Allison Randal wrote:
> 
> include/parrot/interpreter.h:
> Delete the 'struct' before the "PMC *" declaration in the interpreter
> struct.
> 
> -    struct Interp_Context ctx;
> +    struct PMC           *ctx;                /* current Context */
> 
> This lingering bit of old code also appears in src/pmc/lexpad.pmc:
> 
> -        SET_ATTR_ctx(INTERP, SELF, (struct Parrot_Context *)ctx);
> +        SET_ATTR_ctx(INTERP, SELF, (struct PMC *)ctx);

Done.

> 
> Keep backward compatibility with the CONTEXT(interp) macro, by having it 
> return the data struct of the Context PMC. Introduce a new function 
> 'Parrot_pcc_get_current_context' that returns the PMC (it's a clearer 
> name anyway), and we'll deprecate the old macro for 2.0. (General 
> principle: when you're working on refactoring old subsystems, think 
> about how to preserve the old interface until the next deprecation point.)

I tend to disagree with keeping CONTEXT macro backward compatible. 
Parrot_Context structure changed anyway. But I can preserve it (and I 
actually had it before).

> This branch introduces two macros CONTEXT_FIELD and 
> CURRENT_CONTEXT_FIELD. They're an improvement over the old way of 
> directly accessing the context struct members, but they're still too 
> fragile. Since they use the struct field names directly, it ties us to 
> supporting those field names. Instead, create a series of simple API 
> functions for accessing context attributes. Once people are switched 
> over to those, we'll be free to change the internals contexts without 
> needing further API changes. (General principle: when adding a new 
> interface, it should be one we plan to keep.)

They are not part of API. Just quick way to keep branch compiling and 
working. Almost all API accessors functions already implemented.

> 'Parrot_cx_get_caller_ctx' is a good idea, hiding those details behind 
> an interface. But the concurrency scheduler (which is what the 'cx' 
> prefix means) isn't where it belongs. Same for 
> 'Parrot_cx_get_continuation', 'Parrot_cx_dec_recursion_depth', 
> 'Parrot_cx_set_caller_ctx', 'Parrot_cx_get_namespace', 
> 'Parrot_cx_get_pmc_constant', 'Parrot_cx_get_lex_pad'. Put them in 
> src/call/context.c and use the prefix 'pcc' instead of 'cx'. Also make 

I misunderstood "cx" prefix. Done with renaming.

> sure to use "current" in the name of the functions that are fetching or 
> setting the current namespace, context, or continuation, to make it 
> clear what the function is doing.

Technically they are not "current". They always belong to passed Context 
PMC. I can add "current" version of functions which will fetch current 
context from interpreter.

> The function currently named 'Parrot_cx_get_context' takes a Context PMC 
> and returns the data struct for that PMC. We don't want that exposed as 
> an interface, so eliminate it. (CONTEXT(interp) will continue working 
> until after 2.0, so we have a migration strategy for anyone who was 
> relying on the old direct access to the struct. In src/gc/mark_sweep.c, 
> you can just use interp->ctx for the mark the same as all the other PMC 
> members of the interpreter struct.)

I actually think about implementing CONTEXT macro in terms of 
Parrot_pcc_get_context function for simplifying future migration. It's 
easier to put deprecation warning inside function than macro. OTOH may 
be Parrot_pcc_get_context_struct cleaner.


> Don't create a separate header file include/parrot/context.h, since it 
> isn't an independent subsystem. Just add the entries to 
> include/parrot/call.h.

Done.

> src/pmc/context.pmc and src/context.c:
> When you add a new file, just list the current year in the copyright, 
> not 2001-2009.


Done.

> src/pmc/sub.pmc:
> Parrot doesn't use the '//' style of C comments. Also, no generic 'XXX' 
> comments (in src/jit/i386/jit_defs.c too), either flag it with a ticket 
> number or don't flag it at all. See the coding standards for more 
> information.

They left on purpose to track places to fix before merging. Definitely 
will be fixed before.

> Be careful to review all changes when you get ready to merge the branch 
> back in. It looks like your branch reverts some trunk changes in 
> unrelated files (like src/pmc/addrregistry.pmc, src/pmc/class.pmc), 
> which is a sign that there may have been some glitches in your updates 
> of the branch from trunk. The cleanest approach is to create a fresh 
> branch and reapply your changes to it as a patch. That'd be a quick way 
> to filter out the CONTEXT_FIELD/CURRENT_CONTEXT_FIELD changes too and 
> all the changes where you have CONTEXT(interp) returning a PMC (faster 
> than manually undoing them).

I will fully review my changes before merging anyway. On positive note 
branch is clean compiling and passing tests.

> 
> 
> I recommend doing these changes in two passes, one that adds the Context 
> PMC, changes the 'ctx' member of the interpreter to a PMC, introduces 
> accessor functions for the context elements, and makes the minimal set 
> of changes to get the tests passing. Since it leaves the CONTEXT(interp) 
> macro still working, you can skip most of the changes currently in the 
> context_pmc3 branch, so it would be a lightweight branch that could be 
> merged quickly (you could have it ready and merged in a couple of days 
> if you start with a stripped down patch from the current branch).
> 
> Then go back and do a second branch for the weighter conversion of 
> direct struct member access to function calls across all the subsystems.

Actually branch contains both steps.

> The logic in 'Parrot_alloc_context' should move into the PMC, but you 
> can save that for a third refactor. The context-related code in 
> src/sub.c should move into src/call/context.c, but save it for a third 
> or fourth refactor.

Further code reshuffling and function will be done in next branches. I'm 
trying to stay closer to current trunk.

-- 
Bacek