On Tuesday 11 March 2008 21:59:03 [EMAIL PROTECTED] wrote:
> Modified:
> trunk/include/parrot/io.h
> trunk/src/io/io.c
> trunk/src/pmc/parrotio.pmc
>
> Log:
> Fix ParrotIO's clone()
> Add PIO_dup to dup an fd
> Add a set_pmc to ParrotIO
> chromatic++ pmichaud++ Infinoid++
Minor comments, really.
> Modified: trunk/src/io/io.c
> ===========================================================================
>=== --- trunk/src/io/io.c (original)
> +++ trunk/src/io/io.c Tue Mar 11 21:59:02 2008
> @@ -162,6 +162,46 @@
>
> /*
>
> +=item C<ParrotIO * PIO_dup>
> +
> +Duplicates an IO stream.
> +
> +=cut
> +
> +*/
> +
> +PARROT_API
> +PARROT_WARN_UNUSED_RESULT
> +PARROT_CANNOT_RETURN_NULL
> +ParrotIO *
> +PIO_dup(PARROT_INTERP, PMC * pmc)
> +{
> + ParrotIO * const io = PMC_data_typed(pmc, ParrotIO *);
> + ParrotIO * newio;
> + PIOHANDLE newfd = dup(io->fd);
> + ParrotIOLayer * layer = (ParrotIOLayer *)PMC_struct_val(pmc);
I'm sort of wishing for a PMC_struct_val_typed() macro here, but I'd like to
see PMC_struct_val() go away altogether.
Aligning the equals signs makes this code look nicer.
> + if (!layer) {
> + layer = interp->piodata->default_stack;
> + }
> +
> + if (newfd == -1) {
> + real_exception(interp, NULL, 1, "could not dup an fd");
> + }
Curlies unnecessary.
> + newio = PIO_fdopen_down(interp, layer, newfd, io->flags);
A newline would be good here; it indicates a paragraph break in the flow of
the code.
> + /* io could be null here but we still have to
> + * to create a PMC for the caller, no PMCNULL here
> + * as that would cause an exception upon access.
> + */
io or newio? If io is NULL, this code will have segfaulted a few lines ago.
> Modified: trunk/src/pmc/parrotio.pmc
> ===========================================================================
>=== --- trunk/src/pmc/parrotio.pmc (original)
> +++ trunk/src/pmc/parrotio.pmc Tue Mar 11 21:59:02 2008
> @@ -408,11 +408,41 @@
> */
>
> VTABLE PMC *clone() {
> - PMC * const dest = pmc_new(INTERP, SELF->vtable->base_type);
> - PMC_data(dest) = PMC_data(SELF);
> - PMC_struct_val(dest) = PMC_struct_val(SELF);
> + PMC * const dest = PIO_dup(INTERP, SELF);
> return dest;
> }
Might as well just:
return PIO_dup(INTERP, SELF);
> + VTABLE void assign_pmc(PMC *other) {
> + PMC * clone;
This could go in an inner scope (but see later).
> + /* only handle the case where the other PMC is the same type */
> + if (other->vtable->base_type == SELF->vtable->base_type) {
I think this may forbid overriding this PMC type from PIR, but I'm not sure.
> + clone = VTABLE_clone(INTERP, other);
> + PMC_data(SELF) = PMC_data(clone);
> + PMC_struct_val(SELF) = PMC_struct_val(clone);
Any reason not to use STRUCT_COPY()?
A newline is nice here.
> + /* prevent the clone from freeing its data when it's gc'd */
> + PObj_active_destroy_CLEAR(clone);
> + if (PObj_is_PMC_EXT_TEST(clone))
> + clone->pmc_ext = NULL;
> +
> + }
> + else {
> + real_exception(INTERP, NULL, E_TypeError,
> + "Can't assign a non-IO type to an IO");
> + }
> + }
I'd rather handle the exception first, so that it's out of the way and the
rest of the code isn't one nesting level in. Curlies are also unnecessary
here.
-- c
