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
_______________________________________________
http://lists.parrot.org/mailman/listinfo/parrot-dev
