review of context_pmc3 branch

[Allison Randal]

This branch is a part-way step to a larger goal, so this is a sanity 
check that these changes are headed toward the final goal.

The point of a code review is both to keep the code clean and 
maintainable, to make sure we're consistent in coding standards and 
release policy, and to train developers. Please take these comments with 
the intended meaning of "passing on hard-earned knowledge" and not as 
criticism.

A few things to fix:

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


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


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

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

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

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.

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.

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.

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

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.

Allison