[PATCH] Not silently casting away constness in SV macros

[Marcus Holland-Moritz <mhx-perl@gmx.net>]

Change #34600 fixed an erroneously consted SV*. I think it's not
the first time this happened.

The reason why such errors remain unnoticed for a long time is
that a lot of macros (like SvREFCNT_dec()) simply cast away the
constness of the argument, thus inhibiting any compiler warnings.

I've been thinking about a way to do the cast, but nonetheless
get a warning, and the attached patch is what I came up with.

It defines a MUTABLESV() macro like this:

  #if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
  # define MUTABLESV(sv) ({ void *_svm = sv; (SV*) _svm; })
  #else
  # define MUTABLESV(sv) ((SV*) (sv))
  #endif

So, this macro can be used as a replacement for the (SV*) cast.
When using gcc, before actually doing the cast, the argument
is assigned to a void*, which is always possible without an
explicit cast (even in C++). But: if the sv argument is a const
pointer, the compiler will issue a warning for this assignment.

I've tested that it catches the problem fixed with #34600, but
I haven't found any other issues.

I wonder if something like this has been tried before, and if
not, whether this would be useful to apply?

Marcus

-- 
Machines have less problems.  I'd like to be a machine.
		-- Andy Warhol
--- sv.h.orig	2008-10-26 02:47:39.000000000 +0100
+++ sv.h	2008-10-26 03:10:45.000000000 +0100
@@ -218,6 +218,12 @@
 =cut
 */
 
+#if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
+# define MUTABLESV(sv) ({ void *_svm = sv; (SV*) _svm; })
+#else
+# define MUTABLESV(sv) ((SV*) (sv))
+#endif
+
 #define SvANY(sv)	(sv)->sv_any
 #define SvFLAGS(sv)	(sv)->sv_flags
 #define SvREFCNT(sv)	(sv)->sv_refcnt
@@ -225,7 +231,7 @@
 #if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
 #  define SvREFCNT_inc(sv)		\
     ({					\
-	SV * const _sv = (SV*)(sv);	\
+	SV * const _sv = MUTABLESV(sv);	\
 	if (_sv)			\
 	     (SvREFCNT(_sv))++;		\
 	_sv;				\
@@ -238,13 +244,13 @@
     })
 #  define SvREFCNT_inc_NN(sv)		\
     ({					\
-	SV * const _sv = (SV*)(sv);	\
+	SV * const _sv = MUTABLESV(sv);	\
 	SvREFCNT(_sv)++;		\
 	_sv;				\
     })
 #  define SvREFCNT_inc_void(sv)		\
     ({					\
-	SV * const _sv = (SV*)(sv);	\
+	SV * const _sv = MUTABLESV(sv);	\
 	if (_sv)			\
 	    (void)(SvREFCNT(_sv)++);	\
     })
@@ -268,7 +274,7 @@
 #if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
 #  define SvREFCNT_dec(sv)		\
     ({					\
-	SV * const _sv = (SV*)(sv);	\
+	SV * const _sv = MUTABLESV(sv);	\
 	if (_sv) {			\
 	    if (SvREFCNT(_sv)) {	\
 		if (--(SvREFCNT(_sv)) == 0) \
@@ -965,21 +971,21 @@
 #define SvEVALED_off(sv)	(SvFLAGS(sv) &= ~SVrepl_EVAL)
 
 #if defined (DEBUGGING) && defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
-#  define SvVALID(sv)		({ SV *const _svvalid = (SV *) (sv);	\
+#  define SvVALID(sv)		({ const SV *const _svvalid = (const SV *) (sv);    \
 				   if (SvFLAGS(_svvalid) & SVpbm_VALID)	\
 				       assert(!isGV_with_GP(_svvalid));	\
 				   (SvFLAGS(_svvalid) & SVpbm_VALID);	\
 				})
-#  define SvVALID_on(sv)	({ SV *const _svvalid = (SV *) (sv);	\
+#  define SvVALID_on(sv)	({ SV *const _svvalid = MUTABLESV(sv);	\
 				   assert(!isGV_with_GP(_svvalid));	\
 				   (SvFLAGS(_svvalid) |= SVpbm_VALID);	\
 				})
-#  define SvVALID_off(sv)	({ SV *const _svvalid = (SV *) (sv);	\
+#  define SvVALID_off(sv)	({ SV *const _svvalid = MUTABLESV(sv);	\
 				   assert(!isGV_with_GP(_svvalid));	\
 				   (SvFLAGS(_svvalid) &= ~SVpbm_VALID);	\
 				})
 
-#  define SvTAIL(sv)	({ SV *const _svtail = (SV *) (sv);		\
+#  define SvTAIL(sv)	({ const SV *const _svtail = (const SV *) (sv);		\
 			    assert(SvTYPE(_svtail) != SVt_PVAV);		\
 			    assert(SvTYPE(_svtail) != SVt_PVHV);		\
 			    (SvFLAGS(sv) & (SVpbm_TAIL|SVpbm_VALID))	\
@@ -1008,17 +1014,17 @@
 
 #if defined (DEBUGGING) && defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
 #  define SvPAD_TYPED_on(sv)	({					\
-	    SV *const _svpad = (SV *) (sv);				\
+	    SV *const _svpad = MUTABLESV(sv);				\
 	    assert(SvTYPE(_svpad) == SVt_PVMG);				\
 	    (SvFLAGS(_svpad) |= SVpad_NAME|SVpad_TYPED);		\
 	})
 #define SvPAD_OUR_on(sv)	({					\
-	    SV *const _svpad = (SV *) (sv);				\
+	    SV *const _svpad = MUTABLESV(sv);				\
 	    assert(SvTYPE(_svpad) == SVt_PVMG);				\
 	    (SvFLAGS(_svpad) |= SVpad_NAME|SVpad_OUR);			\
 	})
 #define SvPAD_STATE_on(sv)	({					\
-	    SV *const _svpad = (SV *) (sv);				\
+	    SV *const _svpad = MUTABLESV(sv);				\
 	    assert(SvTYPE(_svpad) == SVt_PVNV || SvTYPE(_svpad) == SVt_PVMG); \
 	    (SvFLAGS(_svpad) |= SVpad_NAME|SVpad_STATE);		\
 	})

signature.asc

---

Follow-Ups from:

Nicholas Clark <nick@ccl4.org>

---

[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index][Thread Index][Top&Search][Original]

---