I appreciate the quality of the stm code in general. You're being careful, you know what you're doing with C, and you're good at creating abstractions. I hope when STM is done[*] you'll keep hacking on Parrot.
[*] As if it will ever be really done. "No work of art is ever finished, only abandoned." Here are my comments. Once these issues and/or questions are addressed and/or diposed of, we can merge the state-of-stm onto the trunk. Please correct me wherever I've misunderstood what you're doing. POSSIBLE BUGS * START_WRITE_HLL params vs. usage +#define START_WRITE_HLL(interpreter, hll_info) \ + do { \ + if (PMC_sync(interpreter->HLL_info)) { \ + hll_info = interpreter->HLL_info = \ + Parrot_clone(interpreter, interpreter->HLL_info); \ + if (PMC_sync(interpreter->HLL_info)) { \ + mem_internal_free(PMC_sync(interpreter->HLL_info)); \ + } \ + } \ + } while (0) But then: + START_WRITE_HLL(interpreter, interpreter->HLL_info) That expansion, which is in part: interpreter->HLL_info = interpreter->HLL_info = Parrot_clone(interpreter, interpreter->HLL_info); mem_internal_free(PMC_sync(interpreter->HLL_info)); Looks odd or even broken, offhand. Am I missing something? * enum trailing commas are not standard C89 doesn't allow enum lists to end with a comma. PITA, I know, but we can't require C99 yet. So e.g. 'thread_state_enum' needs a comma removed. QUESTIONS/REQUESTS * You're defining a heck of a lot of macros. Is there any set of them that can be marked as private (to the STM implementation), e.g. with a leading single underscore? Say, if the ATOMIC_INT_CAS macro is only used by other macros and isn't part of the supported interface, then it could be renamed to _ATOMIC_INT_CAS. This will prevent the abstraction from leaking. * It's surely a mean trick on the poor user/programmer to have Parrot_atomic_int actually hold a long. :-, Any reason not to use int? Or do you want Parrot_atomic_long? * Is there any need/use for multiple integral atomic types, like an unsigned or a UINTVAL or something? (I expect the answer is "no".) * Could *_{READ,WRITE}_HLL be named something like *_{READ,WRITE}_HLL_INFO, assuming they need to be macros at all? An HLL is an abstraction that currently doesn't have a single concrete representation. * I see that Parrot_get_HLL_namespace() is now deferring namespace creation. That could perhaps be problematic. We've told users that get_root_namespace ['your_own_hll'; 'x'] and get_hll_namespace ['x'] have identical results. So, what's the payoff for this delayed creation? * Does your ops.num renumber ops, or is diff just getting confused by whitespace? If the former, what's up? CODING STANDARD^WWHIM ISSUES I do need to update pdd07. In the meantime, here are some coding standard issues I'd appreciate if you'd address. For some of them you're going to be setting the new standard for the rest of the code, but such is the fate of code reviewed the guy who's writing the new spec. :-, * Out of all the macros you're defining, will any of them be ever needed by users of the Parrot extension or embedding interfaces? If so, their full official names will have to start with "PARROT_". But when compiling Parrot, it's OK to define non-PARROT_-prefix versions as aliases. For example, if the Parrot_atomic_int is a public type, exposed to embed/extend, as its name implies, then the ATOMIC_* macros will have to be PARROT_ATOMIC_*. But then this is OK too: #ifdef PARROT_IN_CORE # define ATOMIC_FOO PARROT_ATOMIC_FOO # define ATOMIC_BAR PARROT_ATOMIC_BAR ... #endif * macro args & protection Most of your code is really good & careful with macro behaviors. Just a couple exceptions: +#define START_WRITE_HLL(interpreter, hll_info) \ + do { \ + if (PMC_sync(interpreter->HLL_info)) { \ + hll_info = interpreter->HLL_info = \ + Parrot_clone(interpreter, interpreter->HLL_info); \ + if (PMC_sync(interpreter->HLL_info)) { \ + mem_internal_free(PMC_sync(interpreter->HLL_info)); \ + } \ + } \ + } while (0) Macro arguments should be parenthesized when used, and it helps to name them in all caps so as to distinguish them from the normal names ref'd in the macro body. Similarly, when you're not using "do {} while (0)", the macro expansion should be parenthesized if it contains an exposed operator. For example, this: +# define ATOMIC_INT_INC(result, a) result = ++(a).val should be paren'd: +# define ATOMIC_INT_INC(result, a) ((result) = ++(a).val) * useless curlies - if (PMC_IS_NULL(type_hash)) + if (PMC_IS_NULL(type_hash)) { return core_type; + } hash = PMC_struct_val(type_hash); - if (!hash->entries) + if (!hash->entries) { return core_type; + } Something I'm hoping to stamp out is the use of curlies for all if/else clauses, which makes code taller without making it substantially clearer. I'd appreciate if you'd use a no-curlies-when-possible style. OTOH, if you don't want to do this right away, I'd be OK with a merge first, and fixing the curlies later. OTGH, the project needs automated filters for more coding standards, including one that that notes (and optionally kills) the excess curlies. * params of type "const Parrot_Interp" -void pt_thread_prepare_for_run(Parrot_Interp d, Parrot_Interp s); +void pt_thread_prepare_for_run(Parrot_Interp d, const Parrot_Interp s); What's the purpose of this? It doesn't protect *s from changes. [time passes] I now see that some of the existing functions already have this construct. Perhaps you were just imitating them? -- Chip Salzenberg <[EMAIL PROTECTED]>