Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[perl #42151] [PATCH] Assorted casting cleanups - part I

8 views
Skip to first unread message

Steve Peters

unread,
Mar 27, 2007, 12:34:33 PM3/27/07
to bugs-bi...@rt.perl.org
# New Ticket Created by Steve Peters
# Please include the string: [perl #42151]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=42151 >


Index: src/ops/experimental.ops
===================================================================
--- src/ops/experimental.ops (revision 17785)
+++ src/ops/experimental.ops (working copy)
@@ -520,7 +520,7 @@
Interp * const new_interp = (Interp *)PMC_data($1);
opcode_t *pc;
Interp_flags_SET(new_interp, PARROT_EXTERN_CODE_FLAG);
- pc = VTABLE_invoke(new_interp, $2, NULL);
+ pc = (opcode_t *)VTABLE_invoke(new_interp, $2, NULL);
Parrot_runops_fromc_args(new_interp, $2, "P");
goto NEXT();
}
Index: src/ops/object.ops
===================================================================
--- src/ops/object.ops (revision 17785)
+++ src/ops/object.ops (working copy)
@@ -60,7 +60,7 @@
"Method '%Ss' not found", meth);
}
interp->current_object = object;
- interp->current_cont = NEED_CONTINUATION;
+ interp->current_cont = (PMC *)NEED_CONTINUATION;
dest = (opcode_t *)VTABLE_invoke(interp, method_pmc, next);
goto ADDRESS(dest);
}
@@ -76,7 +76,7 @@

next = expr NEXT();
interp->current_object = object;
- interp->current_cont = NEED_CONTINUATION;
+ interp->current_cont = (PMC *)NEED_CONTINUATION;
dest = (opcode_t *)VTABLE_invoke(interp, method_pmc, next);
goto ADDRESS(dest);
}
Index: src/ops/io.ops
===================================================================
--- src/ops/io.ops (revision 17785)
+++ src/ops/io.ops (working copy)
@@ -372,7 +372,7 @@
if (pio->vtable->base_type != enum_class_ParrotIO)
real_exception(interp, NULL, PIO_ERROR,
"Cannot read line from empty filehandle");
- io = PMC_data(pio);
+ io = (ParrotIO *)PMC_data(pio);
if (!io)
real_exception(interp, NULL, PIO_ERROR,
"Cannot read line from empty filehandle");
Index: src/ops/sys.ops
===================================================================
--- src/ops/sys.ops (revision 17785)
+++ src/ops/sys.ops (working copy)
@@ -297,7 +297,7 @@
real_exception(interp, next, NEG_SLEEP,
"Cannot go back in time");
}
- next = Parrot_sleep_on_event(interp, (FLOATVAL) $1, next);
+ next = (opcode_t *)Parrot_sleep_on_event(interp, (FLOATVAL) $1, next);
goto ADDRESS(next);
}

@@ -307,7 +307,7 @@
real_exception(interp, next, NEG_SLEEP,
"Cannot go back in time");
}
- next = Parrot_sleep_on_event(interp, $1, next);
+ next = (opcode_t *)Parrot_sleep_on_event(interp, $1, next);
goto ADDRESS(next);
}

Index: src/charset.c
===================================================================
--- src/charset.c (revision 17785)
+++ src/charset.c (working copy)
@@ -62,7 +62,7 @@
CHARSET *
Parrot_new_charset(Interp *interp)
{
- return mem_sys_allocate(sizeof (CHARSET));
+ return (CHARSET *)mem_sys_allocate(sizeof (CHARSET));
}

void
@@ -184,9 +184,9 @@
* loading of charsets from inside threads
*/
if (!n)
- all_charsets->set = mem_sys_allocate(sizeof (One_charset));
+ all_charsets->set = (One_charset *)mem_sys_allocate(sizeof (One_charset));
else
- all_charsets->set = mem_sys_realloc(all_charsets->set, (n + 1) *
+ all_charsets->set = (One_charset *)mem_sys_realloc(all_charsets->set, (n + 1) *
sizeof (One_charset));
all_charsets->n_charsets++;
all_charsets->set[n].charset = charset;
@@ -219,7 +219,7 @@
CHARSET *charset)
{
if (!all_charsets) {
- all_charsets = mem_sys_allocate(sizeof (All_charsets));
+ all_charsets = (All_charsets *)mem_sys_allocate(sizeof (All_charsets)); all_charsets->n_charsets = 0;
all_charsets->set = NULL;
}
@@ -320,11 +320,11 @@

nc = left->n_converters++;
if (nc) {
- left->to_converters = mem_sys_realloc(left->to_converters,
- sizeof (To_converter) * (nc + 1));
+ left->to_converters = (To_converter *)mem_sys_realloc(
+ left->to_converters, sizeof (To_converter) * (nc + 1)); }
else
- left->to_converters = mem_sys_allocate(sizeof (To_converter));
+ left->to_converters = (To_converter *)mem_sys_allocate(sizeof (To_converter));
left->to_converters[nc].to = rhs;
left->to_converters[nc].func = func;
}
Index: src/exec.c
===================================================================
--- src/exec.c (revision 17785)
+++ src/exec.c (working copy)
@@ -69,7 +69,7 @@
extern PARROT_API int Parrot_exec_rel_count;

Parrot_exec_objfile_t * const obj =
- mem_sys_allocate_zeroed(sizeof (Parrot_exec_objfile_t));
+ (Parrot_exec_objfile_t *)mem_sys_allocate_zeroed(sizeof (Parrot_exec_objfile_t));
exec_init(obj);
Parrot_exec_rel_addr = (char **)mem_sys_allocate_zeroed(4 * sizeof (char *));
obj->bytecode_header_size =
@@ -206,7 +206,7 @@
Parrot_exec_symbol_t *new_symbol;

symbol_number = obj->symbol_count;
- new_symbol = mem_sys_realloc(obj->symbol_table,
+ new_symbol = (Parrot_exec_symbol_t *)mem_sys_realloc(obj->symbol_table, (size_t)(obj->symbol_count + 1) * sizeof (Parrot_exec_symbol_t));
obj->symbol_table = new_symbol;

@@ -266,7 +266,7 @@
extern PARROT_API char **Parrot_exec_rel_addr;
extern PARROT_API int Parrot_exec_rel_count;

- new_relloc = mem_sys_realloc(obj->text_rellocation_table,
+ new_relloc = (Parrot_exec_rellocation_t *)mem_sys_realloc(obj->text_rellocation_table,
(size_t)(obj->text_rellocation_count + 1)
* sizeof (Parrot_exec_rellocation_t));
obj->text_rellocation_table = new_relloc;

Kevin Tew

unread,
Mar 27, 2007, 12:45:33 PM3/27/07
to perl6-i...@perl.org
Looks great Steve,

could (PMC *) get added to the NEED_CONTINUATION macro

Index: include/parrot/sub.h
===================================================================
--- include/parrot/sub.h (revision 17785)
+++ include/parrot/sub.h (working copy)
@@ -109,7 +109,7 @@
* a flag to signal a Sub that a new RetContinuation should be created
*/

-#define NEED_CONTINUATION ((void*)1)
+#define NEED_CONTINUATION ((PMC*)(void*)1)

/*
* Sub and Closure share a Parrot_sub structure.

Kevin

Andy Dougherty

unread,
Mar 27, 2007, 1:52:31 PM3/27/07
to Perl6 Internals
On Tue, 27 Mar 2007, Steve Peters wrote:

> # New Ticket Created by Steve Peters
> # Please include the string: [perl #42151]
> # in the subject line of all future correspondence about this issue.
> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=42151 >

Thanks for taking on this Herculean task. However, one thought immediately
came to mind:

>
> Index: src/ops/experimental.ops
> ===================================================================
> --- src/ops/experimental.ops (revision 17785)
> +++ src/ops/experimental.ops (working copy)
> @@ -520,7 +520,7 @@
> Interp * const new_interp = (Interp *)PMC_data($1);
> opcode_t *pc;
> Interp_flags_SET(new_interp, PARROT_EXTERN_CODE_FLAG);
> - pc = VTABLE_invoke(new_interp, $2, NULL);
> + pc = (opcode_t *)VTABLE_invoke(new_interp, $2, NULL);
> Parrot_runops_fromc_args(new_interp, $2, "P");
> goto NEXT();
> }

Instead of sprinking (opcode_t *) casts everywhere, wouldn't it be better
to declare the invoke() function as returning an (opcode_t *) ?
Similarly for most of the other bits you patched. Wouldn't it make more
sense in the long run to change the functions to say they return what they
actually return?

Of course you can't always do that:

> Parrot_new_charset(Interp *interp)
> {
> - return mem_sys_allocate(sizeof (CHARSET));
> + return (CHARSET *)mem_sys_allocate(sizeof (CHARSET));
> }

mem_sys_allocate() obviously can only return one type, but is used in many
contexts.

--
Andy Dougherty doug...@lafayette.edu

Steve Peters via RT

unread,
Mar 27, 2007, 2:30:59 PM3/27/07
to perl6-i...@perl.org

I'll have to look into some of these a little more closely as I go through. I should probably go
through and refactor as I work on this cleanup. I probably should reject this particular patch
and look for some lower level cleanups that might make this whole process much easier, such
as what you two have pointed out.

> Of course you can't always do that:
>
> > Parrot_new_charset(Interp *interp)
> > {
> > - return mem_sys_allocate(sizeof (CHARSET));
> > + return (CHARSET *)mem_sys_allocate(sizeof (CHARSET));
> > }
>
> mem_sys_allocate() obviously can only return one type, but is used in many
> contexts.
>

Yes, no getting around these ones :)


Andrew Dougherty

unread,
Mar 27, 2007, 3:55:12 PM3/27/07
to Steve Peters via RT
On Tue, 27 Mar 2007, Andy Dougherty wrote:

> I think you'll find a lot of lower-level cleanups are in order.
[other stuff deleted.]

Oops! My previous message was not appropriate, and I didn't mean to send
it. Please ignore it. I'm sorry about that.

--
Andy Dougherty doug...@lafayette.edu

Andy Dougherty

unread,
Mar 27, 2007, 3:39:24 PM3/27/07
to Steve Peters via RT
On Tue, 27 Mar 2007, Steve Peters via RT wrote:

> On Tue Mar 27 10:54:17 2007, doughera wrote:

> > Instead of sprinking (opcode_t *) casts everywhere, wouldn't it be better
> > to declare the invoke() function as returning an (opcode_t *) ?
> > Similarly for most of the other bits you patched. Wouldn't it make more
> > sense in the long run to change the functions to say they return what they
> > actually return?
> >
>
> I'll have to look into some of these a little more closely as I go
> through. I should probably go through and refactor as I work on this
> cleanup. I probably should reject this particular patch and look for
> some lower level cleanups that might make this whole process much
> easier, such as what you two have pointed out.

I think you'll find a lot of lower-level cleanups are in order. Leo
tended to assume all pointers could equally well point anywhere, and often
ignored alignment issues. He often declared stuff (void *) when it really
should have been something more specific. Unfortunately, he also tended
to reject patches to try to clean it up. I wish you better luck.

--
Andy Dougherty doug...@lafayette.edu

Leopold Toetsch

unread,
Mar 27, 2007, 6:41:24 PM3/27/07
to perl6-i...@perl.org, Andy Dougherty
Am Dienstag, 27. März 2007 19:52 schrieb Andy Dougherty:
> Instead of sprinking (opcode_t *) casts everywhere, wouldn't it be better
> to declare the invoke() function as returning an (opcode_t *) ?  

True. But while invoke() et al receive and return an (opcode_t*) the actual
running runloop (with e.g. predereferenced runcores) wants a void**, which
should OTOH not need a type cast.

E.g. in src/ops/core_ops_cgp.c

/* run_core_func_decl - tools/build/ops2c.pl -> Parrot::OpTrans::CPrederef */
void ** cgp_core(void **cur_op, Parrot_Interp interp)

The usage of properly declared void** op_ptrs inside that shouldn't trigger
much need for casting as opcode_t*.

leo

Leopold Toetsch

unread,
Mar 27, 2007, 6:42:15 PM3/27/07
to perl6-i...@perl.org, Andy Dougherty, Steve Peters via RT
Am Dienstag, 27. März 2007 21:39 schrieb Andy Dougherty:
>  Leo
> tended to assume all pointers could equally well point anywhere, and often
> ignored alignment issues.

Nope. Sorry.

leo

Leopold Toetsch

unread,
Mar 27, 2007, 7:08:40 PM3/27/07
to perl6-i...@perl.org, Andrew Dougherty, Steve Peters via RT
Am Dienstag, 27. März 2007 21:55 schrieb Andrew Dougherty:
> On Tue, 27 Mar 2007, Andy Dougherty wrote:
> > I think you'll find a lot of lower-level cleanups are in order.
>
> [other stuff deleted.]
>
> Oops! My previous message was not appropriate, and I didn't mean to send
> it. Please ignore it. I'm sorry about that.

It looks to me that I've already replied to that cancelled message. Therefore
another reply is maybe in order, which is a minimal summary of a quick grep
of old mails:

Date: Thu, 02 Jun 2005 23:18:56 +0200
---
Andy Dougherty wrote:

> Here's the Solaris patch:

BTW it seems that you don't have ci privs for Parrot, which IMHO is just
a blunder.

If you like to commit that stuff yourself please send me a mail with
your auth.perl.org account name and I'll forward it to Robert.

Thanks,
leo

---

Subject: svn access (was: [perl #38576] [PATCH] Make DETAIL_MEMORY_DEBUG
work. )

On Feb 15, 2006, at 18:53, Andy Dougherty (via RT) wrote:

> # New Ticket Created by Andy Dougherty

Andy, I've already asked once: don't you have svn access? If no (and if
you want it) please mail me your auth.perl.org account data, to get you
svn priv bits.
If yes, I'd really prefer that such patches just go in. And honestly
all your patches are well-thought and hight quality.

Thanks,
leo

Steve Peters via RT

unread,
Mar 29, 2007, 10:29:37 AM3/29/07
to perl6-i...@perl.org
On Tue Mar 27 10:54:17 2007, doughera wrote:


Of course, some well defined macros could assist in cleaning this up.
For example...

#define PARROT_MEM_ALLOCATE(type) \
(type *)mem_sys_allocate(sizeof(type))

I don't know the Parrot opinion of macros, but it would certainly make
the code cleanup much easier.

Chromatic

unread,
Mar 29, 2007, 1:51:30 PM3/29/07
to perl6-i...@perl.org, parrotbug...@parrotcode.org
On Thursday 29 March 2007 07:29, Steve Peters via RT wrote:

> Of course, some well defined macros could assist in cleaning this up.
> For example...
>
> #define PARROT_MEM_ALLOCATE(type) \
> (type *)mem_sys_allocate(sizeof(type))
>
> I don't know the Parrot opinion of macros, but it would certainly make
> the code cleanup much easier.

That wouldn't hurt my feelings. It really wouldn't hurt my feelings to use a
macro for PARROT_MEM_FREE either; then it would be trivial to switch to a
memory poisoning scheme that hopefully generates segfaults much, much
earlier.

-- c

0 new messages