Re: [perl #56110] [PATCH] Warnings on Solaris
On Wednesday 09 July 2008 08:27:35 Andrew Johnson wrote: > > Done in r29179, please confirm that gives no warning now. > > Confirmed, those warnings have gone. I'm still getting loads of "warning: > statement not reached" but I'll work out how to suppress those and post a > fix separately. Does your compiler have a function annotation which means "This function does not return"? If so, we can add a probe and a define for that. -- c
Re: [perl #56110] [PATCH] Warnings on Solaris
On Tuesday 08 July 2008 17:55:42 NotFound wrote: > On Mon, Jun 23, 2008 at 4:37 PM, Andrew Johnson <[EMAIL PROTECTED]> wrote: > >> Here is the patch. It avoids the warning both in C and C++ with gcc. > > > > Works fine for me, no warning. > > It might be worth adding a comment into parrot.h to clarify that > > PARROT_const_cast should *only* be used for c/v qualifier changes, and > > that additional casting may be necessary. > > Done in r29179, please confirm that gives no warning now. Confirmed, those warnings have gone. I'm still getting loads of "warning: statement not reached" but I'll work out how to suppress those and post a fix separately. - Andrew -- Talk is cheap. Show me the code. -- Linus Torvalds
Re: [perl #56110] [PATCH] Warnings on Solaris
On Mon, Jun 23, 2008 at 4:37 PM, Andrew Johnson <[EMAIL PROTECTED]> wrote: >> Here is the patch. It avoids the warning both in C and C++ with gcc. > Works fine for me, no warning. > It might be worth adding a comment into parrot.h to clarify that > PARROT_const_cast should *only* be used for c/v qualifier changes, and that > additional casting may be necessary. Done in r29179, please confirm that gives no warning now. -- Salu2
Re: [perl #56110] [PATCH] Warnings on Solaris
On Monday 23 June 2008 09:08:07 NotFound wrote: > Here is the patch. It avoids the warning both in C and C++ with gcc. Works fine for me, no warning. It might be worth adding a comment into parrot.h to clarify that PARROT_const_cast should *only* be used for c/v qualifier changes, and that additional casting may be necessary. - Andrew -- Talk is cheap. Show me the code. -- Linus Torvalds
Re: [perl #56110] [PATCH] Warnings on Solaris
On Mon, Jun 23, 2008 at 4:37 PM, Andrew Johnson <[EMAIL PROTECTED]> wrote: > It might be worth adding a comment into parrot.h to clarify that > PARROT_const_cast should *only* be used for c/v qualifier changes, and that > additional casting may be necessary. Yes, but I don't know how to write it without a long explanation of the rationale that avoids people takes it as an invitation to add more casts, 'just in case'. I like better to have these warnings and carefully review each case than to hide them. Maybe a comment that says 'Check each usage of this macro with a C++ build' will be a good trade-off. -- Salu2
Re: [perl #56110] [PATCH] Warnings on Solaris
Here is the patch. It avoids the warning both in C and C++ with gcc.
--
Salu2
Index: src/packfile.c
===
--- src/packfile.c (revisión: 28668)
+++ src/packfile.c (copia de trabajo)
@@ -348,7 +348,7 @@
#ifdef PARROT_HAS_HEADER_SYSMMAN
if (pf->is_mmap_ped) {
DECL_CONST_CAST;
-munmap(PARROT_const_cast(opcode_t *, pf->src), pf->size);
+munmap((void *)PARROT_const_cast(opcode_t *, pf->src), pf->size);
}
#endif
@@ -875,7 +875,7 @@
if (self->is_mmap_ped
&& (self->need_endianize || self->need_wordsize)) {
DECL_CONST_CAST;
-munmap(PARROT_const_cast(opcode_t *, self->src), self->size);
+munmap((void *)PARROT_const_cast(opcode_t *, self->src), self->size);
self->is_mmap_ped = 0;
}
#endif
Re: [perl #56110] [PATCH] Warnings on Solaris
On Sat, Jun 21, 2008 at 11:10 AM, chromatic <[EMAIL PROTECTED]> wrote: > On Friday 20 June 2008 10:53:55 Andrew Johnson wrote: > >> I'm concluding that these warnings are due to incorrect casting inside of >> packfile.c. >> >> In both cases, the code generating the warnings looks like this: >> >> munmap(PARROT_const_cast(opcode_t *, pf->src), pf->size); >> >> pf->src is a const opcode_t* and the purpose of the PARROT_const_cast is to >> drop the const. The first argument to munmap() is a void* on Linux and Mac, >> but a char* on Solaris; I tried globally defining the feature-test macros >> _POSIX_C_SOURCE or _XPG4_2, but these are supposed to be an all-or-nothing >> choice, and in both cases they broke the build elsewhere. >> >> In C any pointer auto-converts to a void*, but has to be cast to any other >> pointer type such as a char*. If we change the above casts to void* the >> warning on Solaris goes away. I have checked this on Linux and it builds >> fine. Hence the attached patch to packfile.c > > This sounds reasonable. I'd like to hear results from trying to compile with > a C++ compiler though. NotFound, any luck? The goal of the PARROT_const_cast macro is to do the const-dropping casting (given that is almost impossible to completely get rid of all of them) in the safer and less annoying (for the people building and searching errors, not for the writer) possible way. To accomplish that goal, when compiling with C++ is mappped to a c++ const_cast, and because of that the type converted to can only differ to the argument type in constness and volatileness. In several cases the desired conversion is to void *, but that is not problem because after dropping the const the conversion to void * is done automatically without warnings. In corner cases like this, the only solution I see is to explicitly cast to (void *) the result of PARROT_const_cast, like this: munmap( (void *)PARROT_const_cast(opcode_t *, pf->src), pf->size); I think that the goal is good enough to justify the need to use such ugly casts occasionally. Can't write a fixed patch at this moment, I will send it later today. -- Salu2
Re: [perl #56110] [PATCH] Warnings on Solaris
On Friday 20 June 2008 10:53:55 Andrew Johnson wrote: > I'm concluding that these warnings are due to incorrect casting inside of > packfile.c. > > In both cases, the code generating the warnings looks like this: > > munmap(PARROT_const_cast(opcode_t *, pf->src), pf->size); > > pf->src is a const opcode_t* and the purpose of the PARROT_const_cast is to > drop the const. The first argument to munmap() is a void* on Linux and Mac, > but a char* on Solaris; I tried globally defining the feature-test macros > _POSIX_C_SOURCE or _XPG4_2, but these are supposed to be an all-or-nothing > choice, and in both cases they broke the build elsewhere. > > In C any pointer auto-converts to a void*, but has to be cast to any other > pointer type such as a char*. If we change the above casts to void* the > warning on Solaris goes away. I have checked this on Linux and it builds > fine. Hence the attached patch to packfile.c This sounds reasonable. I'd like to hear results from trying to compile with a C++ compiler though. NotFound, any luck? -- c
Re: [perl #56110] [PATCH] Warnings on Solaris
On Thursday 19 June 2008 15:33:14 Parrot via RT wrote:
> "src/packfile.c", line 351: warning: argument #1 is incompatible with
> prototype:
> prototype: pointer to char : "/usr/include/sys/mman.h", line 161
> argument : pointer to long
> "src/packfile.c", line 878: warning: argument #1 is incompatible with
> prototype:
> prototype: pointer to char : "/usr/include/sys/mman.h", line 161
> argument : pointer to long
I'm concluding that these warnings are due to incorrect casting inside of
packfile.c.
In both cases, the code generating the warnings looks like this:
munmap(PARROT_const_cast(opcode_t *, pf->src), pf->size);
pf->src is a const opcode_t* and the purpose of the PARROT_const_cast is to
drop the const. The first argument to munmap() is a void* on Linux and Mac,
but a char* on Solaris; I tried globally defining the feature-test macros
_POSIX_C_SOURCE or _XPG4_2, but these are supposed to be an all-or-nothing
choice, and in both cases they broke the build elsewhere.
In C any pointer auto-converts to a void*, but has to be cast to any other
pointer type such as a char*. If we change the above casts to void* the
warning on Solaris goes away. I have checked this on Linux and it builds
fine. Hence the attached patch to packfile.c
- Andrew
--
Talk is cheap. Show me the code. -- Linus Torvalds
Index: src/packfile.c
===
--- src/packfile.c (revision 28514)
+++ src/packfile.c (working copy)
@@ -348,7 +348,7 @@
#ifdef PARROT_HAS_HEADER_SYSMMAN
if (pf->is_mmap_ped) {
DECL_CONST_CAST;
-munmap(PARROT_const_cast(opcode_t *, pf->src), pf->size);
+munmap(PARROT_const_cast(void *, pf->src), pf->size);
}
#endif
@@ -875,7 +875,7 @@
if (self->is_mmap_ped
&& (self->need_endianize || self->need_wordsize)) {
DECL_CONST_CAST;
-munmap(PARROT_const_cast(opcode_t *, self->src), self->size);
+munmap(PARROT_const_cast(void *, self->src), self->size);
self->is_mmap_ped = 0;
}
#endif
