Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for Ticket #1215 #129

Closed
wants to merge 2 commits into from

2 participants

@soh-cah-toa

This is the refactored code for ticket #1215. I took the duplicate code in the 'fetch' and 'vivify' opcodes and moved it into separate functions to reduce redundancy.

I realize I probably should have just submitted a patch through Trac but I had already pushed the changes when I needed some help earlier today.

@cotto

Thanks for contributing. The common code between the ops can be factored into a single function, but it's not a general-purpose function we should expose as part of the API. Please make the following changes:

  • Don't change unrelated POD. This looks unintentional but still needs to be fixed. The codingstd_tests makefile target catches this.

  • Break the common code into a single static function in the .ops file. You can surround it with BEGIN_OPS_PREAMBLE and END_OPS_PREAMBLE and it'll be inserted into the generated C code. I'd suggest the name "parrot_pmc_new_from_type".

  • Use return values instead of ARGMOD arguments. The PMC null checks should stay in the ops.

@soh-cah-toa

Alright, will do. Someone on #parrot (I forget who) said I should put it in src/pmc.c for now; that's why it's there.

@soh-cah-toa soh-cah-toa reopened this
@cotto

discussion moved to trac; closing this request

@cotto cotto closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2011
  1. @soh-cah-toa

    Fix for the issues addressed in ticket #1215. Refactored duplicate co…

    soh-cah-toa authored
    …de in the 'fetch' and 'vivify' opcodes.
This page is out of date. Refresh to see the latest.
Showing with 110 additions and 107 deletions.
  1. +18 −0 include/parrot/pmc.h
  2. +26 −107 src/ops/experimental.ops
  3. +66 −0 src/pmc.c
View
18 include/parrot/pmc.h
@@ -189,6 +189,17 @@ INTVAL Parrot_pmc_type_does(PARROT_INTERP,
__attribute__nonnull__(1)
__attribute__nonnull__(2);
+void Parrot_stock_fetch(PARROT_INTERP,
+ ARGIN_NULLOK(PMC *pmc),
+ ARGIN(PMC *key))
+ __attribute__nonnull__(1)
+ __attribute__nonnull__(3);
+
+void Parrot_stock_vivify(PARROT_INTERP, ARGIN(PMC *pmc), ARGIN(PMC *key))
+ __attribute__nonnull__(1)
+ __attribute__nonnull__(2)
+ __attribute__nonnull__(3);
+
#define ASSERT_ARGS_Parrot_pmc_box_c_string_array __attribute__unused__ int _ASSERT_ARGS_CHECK = (\
PARROT_ASSERT_ARG(interp) \
, PARROT_ASSERT_ARG(s))
@@ -259,6 +270,13 @@ INTVAL Parrot_pmc_type_does(PARROT_INTERP,
#define ASSERT_ARGS_Parrot_pmc_type_does __attribute__unused__ int _ASSERT_ARGS_CHECK = (\
PARROT_ASSERT_ARG(interp) \
, PARROT_ASSERT_ARG(role))
+#define ASSERT_ARGS_Parrot_stock_fetch __attribute__unused__ int _ASSERT_ARGS_CHECK = (\
+ PARROT_ASSERT_ARG(interp) \
+ , PARROT_ASSERT_ARG(key))
+#define ASSERT_ARGS_Parrot_stock_vivify __attribute__unused__ int _ASSERT_ARGS_CHECK = (\
+ PARROT_ASSERT_ARG(interp) \
+ , PARROT_ASSERT_ARG(pmc) \
+ , PARROT_ASSERT_ARG(key))
/* Don't modify between HEADERIZER BEGIN / HEADERIZER END. Your changes will be lost. */
/* HEADERIZER END: src/pmc.c */
View
133 src/ops/experimental.ops
@@ -29,7 +29,7 @@ all generated ops files.
=over 4
-=item B<trap>()
+=item C<trap>()
Break into debugger. Implementation notes:
@@ -50,12 +50,12 @@ op trap() :deprecated {
#endif
}
-=item B<set_label>(invar PMC, inconst LABEL)
+=item C<set_label(invar PMC, inconst LABEL)>
Sets the opcode_t* label value for the given PMC. This is basically only
useful for PMCs such as Sub, Continuation, ExceptionHandler and derivatives
-=item B<get_label>(out INT, invar PMC)
+=item C<get_label(out INT, invar PMC)>
Gets the opcode_t* label value from the given PMC. This is basically only
useful for PMCs such as Sub, Continuation, ExceptionHandler and derivatives
@@ -71,85 +71,43 @@ inline op get_label(out INT, invar PMC) {
$1 = PTR2INTVAL(ptr);
}
-=item B<fetch>(out PMC, in PMC, in PMC, in PMC)
+=item C<fetch(out PMC, in PMC, in PMC, in PMC)>
-Fetches a value from $2, keyed by $3 into $1. If the resulting PMC is PMCNULL,
-uses the type in $4 to create and return a new PMC.
+=item C<fetch(out PMC, in PMC, in INT, in PMC)>
-=item B<fetch>(out PMC, in PMC, in INT, in PMC)
+=item C<fetch(out PMC, in PMC, in STR, in PMC)>
-=item B<fetch>(out PMC, in PMC, in STR, in PMC)
+Fetches a value from $2, keyed by $3 into $1. If the resulting PMC is PMCNULL,
+uses the type in $4 to create and return a new PMC.
=cut
inline op fetch(out PMC, in PMC, in PMC, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed(interp, $2, $3);
- if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
- }
+
+ Parrot_stock_fetch(interp, $1, $4);
}
inline op fetch(out PMC, in PMC, in INT, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed_int(interp, $2, $3);
- if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
- }
+
+ Parrot_stock_fetch(interp, $1, $4);
}
inline op fetch(out PMC, in PMC, in STR, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed_str(interp, $2, $3);
- if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
- }
+
+ Parrot_stock_fetch(interp, $1, $4);
}
-=item B<vivify>(out PMC, in PMC, in PMC, in PMC)
+=item C<vivify(out PMC, in PMC, in PMC, in PMC)>
-Fetches a value from $2, keyed by $3 into $1. If the resulting PMC is PMCNULL,
-uses the type in $4 to create and return a new PMC.
+=item C<vivify(out PMC, in PMC, in INT, in PMC)>
-=item B<vivify>(out PMC, in PMC, in INT, in PMC)
+=item C<vivify(out PMC, in PMC, in STR, in PMC)>
-=item B<vivify>(out PMC, in PMC, in STR, in PMC)
+Fetches a value from $2, keyed by $3 into $1. If the resulting PMC is PMCNULL,
+uses the type in $4 to create and return a new PMC.
=cut
@@ -157,21 +115,8 @@ inline op vivify(out PMC, in PMC, in PMC, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed(interp, $2, $3);
if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
-
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
+ Parrot_stock_vivify(interp, $1, $4);
+
VTABLE_set_pmc_keyed(interp, $2, $3, $1);
}
}
@@ -180,20 +125,7 @@ inline op vivify(out PMC, in PMC, in INT, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed_int(interp, $2, $3);
if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
+ Parrot_stock_vivify(interp, $1, $4);
VTABLE_set_pmc_keyed_int(interp, $2, $3, $1);
}
@@ -203,28 +135,15 @@ inline op vivify(out PMC, in PMC, in STR, in PMC) :base_core {
$1 = VTABLE_get_pmc_keyed_str(interp, $2, $3);
if (PMC_IS_NULL($1)) {
- PMC * const classobj = Parrot_oo_get_class(interp, $4);
- if (!PMC_IS_NULL(classobj))
- $1 = VTABLE_instantiate(interp, classobj, PMCNULL);
- else {
- const INTVAL type = Parrot_pmc_get_type(interp, $4);
- if (type <= 0) {
- opcode_t *dest = Parrot_ex_throw_from_op_args(
- interp, expr NEXT(), EXCEPTION_NO_CLASS,
- "Class '%Ss' not found", VTABLE_get_repr(interp, $4));
- goto ADDRESS(dest);
- }
-
- $1 = Parrot_pmc_new(interp, type);
- }
+ Parrot_stock_vivify(interp, $1, $4);
VTABLE_set_pmc_keyed_str(interp, $2, $3, $1);
}
}
-=item B<new>(out PMC, in STR, in INT)
+=item C<new(out PMC, in STR, in INT)>
-=item B<new>(out PMC, in PMC, in INT)
+=item C<new(out PMC, in PMC, in INT)>
=cut
@@ -284,7 +203,7 @@ op new(out PMC, in PMC, in INT) {
}
}
-=item B<root_new>(out PMC, in PMC, in INT)
+=item C<root_new(out PMC, in PMC, in INT)>
=cut
View
66 src/pmc.c
@@ -1132,6 +1132,72 @@ Parrot_pmc_type_does(PARROT_INTERP, ARGIN(const STRING *role), INTVAL type)
/*
+=item C<void Parrot_stock_fetch(PARROT_INTERP, PMC *pmc, PMC *key)>
+
+Called by the C<fetch> opcode. Should NOT be called directly. It's only
+purpose is to eliminate duplicate code in C<fetch>, hence the code is
+"stock". Think of "stock" as in stock merchandise.
+
+*/
+
+void
+Parrot_stock_fetch(PARROT_INTERP, ARGIN_NULLOK(PMC *pmc), ARGIN(PMC *key))
+{
+ ASSERT_ARGS(Parrot_stock_fetch)
+
+ if (PMC_IS_NULL(pmc)) {
+ PMC * const classobj = Parrot_oo_get_class(interp, key);
+
+ if (!PMC_IS_NULL(classobj))
+ pmc = VTABLE_instantiate(interp, classobj, PMCNULL);
+ else {
+ const INTVAL type = Parrot_pmc_get_type(interp, key);
+
+ if (type <= 0) {
+ Parrot_ex_throw_from_c_args(interp, NULL,
+ EXCEPTION_NO_CLASS, "Class '%Ss' not found",
+ VTABLE_get_repr(interp, key));
+ }
+
+ pmc = Parrot_pmc_new(interp, type);
+ }
+ }
+}
+
+/*
+
+=item C<void Parrot_stock_vivify(PARROT_INTERP, PMC *pmc, PMC *key)>
+
+Called by the C<vivify> opcode. Should NOT be called directly. It's only
+purpose is to eliminate duplicate code in C<vivify>, hence the code is
+"stock". Think of "stock" as in stock merchandise.
+
+*/
+
+void
+Parrot_stock_vivify(PARROT_INTERP, ARGIN(PMC *pmc), ARGIN(PMC *key))
+{
+ ASSERT_ARGS(Parrot_stock_vivify)
+
+ PMC * const classobj = Parrot_oo_get_class(interp, key);
+
+ if (!PMC_IS_NULL(classobj))
+ pmc = VTABLE_instantiate(interp, classobj, PMCNULL);
+ else {
+ const INTVAL type = Parrot_pmc_get_type(interp, key);
+
+ if (type <= 0) {
+ Parrot_ex_throw_from_c_args(interp, NULL,
+ EXCEPTION_NO_CLASS, "Class '%Ss' not found",
+ VTABLE_get_repr(interp, key));
+ }
+
+ pmc = Parrot_pmc_new(interp, type);
+ }
+}
+
+/*
+
=back
=head1 SEE ALSO
Something went wrong with that request. Please try again.