Re: Selectively suppress -Wcast-function-type (issue 357770043 by address@hidden)

[nine . fierce . ballads]

Reviewers: dak,

Message:
On 2018/08/13 07:11:38, dak wrote:
> I'd use scm_func_cast<scm_t_blabla> (actual_func)

I'll do that.

> Any chance that an implementation via a double cast (with something
like void *
> in between) will work?  Strictly speaking that's worse than casting
from one
> function pointer to another since function and data pointers need not
be the
> same size on segmented architectures, but lots of code does stuff like
that
> anyway.

The first thing I tried was an intermediate (void*), and that did
eliminate the warning.  The reason you cite is the reason I didn't
propose it, but if you're comfortable with it, I'll do it.  Interaction
with Guile is not a place where I care to assert my preferences.

Description:
https://sourceforge.net/p/testlilyissues/issues/5400/

A simple reinterpret_cast<> was not enough to suppress the warning that
g++ 8 produces in these cases.  This was the next approach I could think
of.

Would it be preferred to define a single templated function for this
thing, e.g. reinterpret_function<scm_t_subr> (smob_p)?  I chose to
define two separate functions rather than think very hard about what to
name a single function.  Suggestions are welcome.

Please review this at https://codereview.appspot.com/357770043/

Affected files (+47, -9 lines):
  M lily/dispatcher.cc
  M lily/general-scheme.cc
  M lily/include/lily-guile.hh
  M lily/include/lily-guile-macros.hh
  M lily/include/smobs.hh
  M lily/include/smobs.tcc
  M lily/ly-module.cc
  M lily/module-scheme.cc
  M lily/scm-hash.cc


Index: lily/dispatcher.cc
diff --git a/lily/dispatcher.cc b/lily/dispatcher.cc
index  
b1b076759f83b8a7e1a655d84054957b84afb4c5..bb32bc8afbca80511e9790d833f523bbea865036  
100644
--- a/lily/dispatcher.cc
+++ b/lily/dispatcher.cc
@@ -192,7 +192,7 @@ accumulate_types (void * /* closure */,
 SCM
 Dispatcher::listened_types ()
 {
-  return scm_internal_hash_fold ((scm_t_hash_fold_fn) &accumulate_types,
+  return scm_internal_hash_fold (to_scm_t_hash_fold_fn (accumulate_types),
                                  NULL, SCM_EOL, listeners_);
 }

Index: lily/general-scheme.cc
diff --git a/lily/general-scheme.cc b/lily/general-scheme.cc
index  
d25e6c9ef84b0418edc6dc8bf461c88fa3595e3c..8d39576d96835b068bb45d5354fde6206dd15c83  
100644
--- a/lily/general-scheme.cc
+++ b/lily/general-scheme.cc
@@ -409,7 +409,7 @@ LY_DEFINE (ly_hash_table_keys, "ly:hash-table-keys",
            1, 0, 0, (SCM tab),
            "Return a list of keys in @var{tab}.")
 {
-  return scm_internal_hash_fold ((scm_t_hash_fold_fn) &accumulate_symbol,
+  return scm_internal_hash_fold (to_scm_t_hash_fold_fn (accumulate_symbol),
                                  NULL, SCM_EOL, tab);
 }

Index: lily/include/lily-guile-macros.hh
diff --git a/lily/include/lily-guile-macros.hh  
b/lily/include/lily-guile-macros.hh
index  
201b5cfc772207d56543d797b6cf485dbc4b7c8c..341b28f93ff19a4405a0121cf6c12e7b4574c56b  
100644
--- a/lily/include/lily-guile-macros.hh
+++ b/lily/include/lily-guile-macros.hh
@@ -58,6 +58,24 @@
 typedef SCM (*scm_t_subr) (GUILE_ELLIPSIS);
 #endif

+template <typename FunctionPtr>
+inline scm_t_subr to_scm_t_subr(const FunctionPtr& f)
+{
+#if __GNUC__ >= 8
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wcast-function-type"
+#endif
+
+  // GCC 8 warns about this because the result of casting a function  
pointer to
+  // an incompatible type and then calling it is undefined.  Casting it  
back
+  // before calling it is fine, so hopefully Guile will do that.
+  return reinterpret_cast<scm_t_subr> (f);
+
+#if __GNUC__ >= 8
+#pragma GCC diagnostic pop
+#endif
+}
+
 /* Unreliable with gcc-2.x
    FIXME: should add check for x86 as well?  */
 #define CACHE_SYMBOLS
@@ -118,7 +136,7 @@ string predicate_to_typename (void *ptr);
     string id = mangle_cxx_identifier (cxx); \
     TYPE ::FUNC ## _proc = scm_c_define_gsubr  
(id.c_str(),                      \
                                                (ARGCOUNT-OPTIONAL_COUNT),  
OPTIONAL_COUNT, 0,    \
-                                               (scm_t_subr) TYPE::FUNC); \
+                                               to_scm_t_subr  
(TYPE::FUNC)); \
     ly_add_function_documentation (TYPE :: FUNC ## _proc, id.c_str(), "", \
                                    DOC);                                \
     scm_c_export (id.c_str (), NULL);                                   \
@@ -156,7 +174,7 @@ void ly_check_name (const string &cxx, const string  
&fname);
   INITPREFIX ## init ()                                                 \
   {                                                                     \
     FNAME ## _proc = scm_c_define_gsubr (PRIMNAME, REQ, OPT, VAR,       \
-                                         (scm_t_subr) FNAME); \
+                                         to_scm_t_subr (FNAME));        \
     ly_check_name (#FNAME, PRIMNAME);\
     ly_add_function_documentation (FNAME ## _proc, PRIMNAME, #ARGLIST,  \
                                    DOCSTRING);                          \
Index: lily/include/lily-guile.hh
diff --git a/lily/include/lily-guile.hh b/lily/include/lily-guile.hh
index  
d1ca0424bb3e0df24d958260bedc89f462cf02aa..113486c99ad2271cfc1025dd067de1fe833ed12d  
100644
--- a/lily/include/lily-guile.hh
+++ b/lily/include/lily-guile.hh
@@ -221,4 +221,22 @@ typedef SCM (*scm_t_hash_fold_fn) (GUILE_ELLIPSIS);
 typedef SCM (*scm_t_hash_handle_fn) (GUILE_ELLIPSIS);
 #endif

+template <typename FunctionPtr>
+inline scm_t_hash_fold_fn to_scm_t_hash_fold_fn(const FunctionPtr& f)
+{
+#if __GNUC__ >= 8
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wcast-function-type"
+#endif
+
+  // GCC 8 warns about this because the result of casting a function  
pointer to
+  // an incompatible type and then calling it is undefined.  Casting it  
back
+  // before calling it is fine, so hopefully Guile will do that.
+  return reinterpret_cast<scm_t_hash_fold_fn> (f);
+
+#if __GNUC__ >= 8
+#pragma GCC diagnostic pop
+#endif
+}
+
 #endif /* LILY_GUILE_HH */
Index: lily/include/smobs.hh
diff --git a/lily/include/smobs.hh b/lily/include/smobs.hh
index  
00217866dc07dc92eff400e23050d50b46f67a3b..67d9699c5ab3fb84bcdf959eda110c5cf09d82b3  
100644
--- a/lily/include/smobs.hh
+++ b/lily/include/smobs.hh
@@ -225,10 +225,11 @@ private:
   static void smob_proc_init (scm_t_bits smob_tag)                      \
   {                                                                     \
     scm_set_smob_apply (smob_tag,                                       \
-                        (scm_t_subr)smob_trampoline<PMF>,               \
+                        to_scm_t_subr (smob_trampoline<PMF>),           \
                         REQ, OPT, VAR);                                 \
   }

+protected:
   // Well, function template argument packs are a C++11 feature.  So
   // we just define a bunch of trampolines manually.  It turns out
   // that GUILE 1.8.8 cannot actually make callable structures with
@@ -257,6 +258,7 @@ private:
     return (Super::unchecked_unsmob (self)->*pmf)(arg1, arg2, arg3);
   }

+private:
   static bool is_smob (SCM s)
   {
     return SCM_SMOB_PREDICATE (smob_tag (), s);
Index: lily/include/smobs.tcc
diff --git a/lily/include/smobs.tcc b/lily/include/smobs.tcc
index  
818c0900a56bac154476b49282eae6bf529241b8..04e9e6ac5bafc4b3c800de960bc5412e29e307f0  
100644
--- a/lily/include/smobs.tcc
+++ b/lily/include/smobs.tcc
@@ -144,7 +144,7 @@ void Smob_base<Super>::init ()
   if (Super::type_p_name_ != 0)
     {
       SCM subr = scm_c_define_gsubr (Super::type_p_name_, 1, 0, 0,
-                                     (scm_t_subr) smob_p);
+                                     to_scm_t_subr (smob_p));
       string fundoc = string("Is @var{x} a @code{") + smob_name_
         + "} object?";
       ly_add_function_documentation (subr, Super::type_p_name_, "(SCM x)",
Index: lily/ly-module.cc
diff --git a/lily/ly-module.cc b/lily/ly-module.cc
index  
2076168fda75ac9ad9c73004d611bb24ebc881af..41c1b6ccf9d83278da6d98a6fb897b7f8686e918  
100644
--- a/lily/ly-module.cc
+++ b/lily/ly-module.cc
@@ -108,7 +108,7 @@ LY_DEFINE (ly_module_2_alist, "ly:module->alist",
   SCM_VALIDATE_MODULE (1, mod);
   SCM obarr = SCM_MODULE_OBARRAY (mod);

-  return scm_internal_hash_fold ((scm_t_hash_fold_fn) &entry_to_alist,
+  return scm_internal_hash_fold (to_scm_t_hash_fold_fn (entry_to_alist),
                                  NULL, SCM_EOL, obarr);
 }

Index: lily/module-scheme.cc
diff --git a/lily/module-scheme.cc b/lily/module-scheme.cc
index  
eda467fe68b259336663f08ccfaf72f1e2818afb..bcd3338f7e08407f83b21eb24d5e1ce7cd7f11e7  
100644
--- a/lily/module-scheme.cc
+++ b/lily/module-scheme.cc
@@ -45,7 +45,7 @@ LY_DEFINE (ly_module_copy, "ly:module-copy",
 {
 #define FUNC_NAME __FUNCTION__
   SCM_VALIDATE_MODULE (1, src);
-  scm_internal_hash_fold ((scm_t_hash_fold_fn) &module_define_closure_func,
+  scm_internal_hash_fold (to_scm_t_hash_fold_fn  
(module_define_closure_func),
                           static_cast<void *> (&dest),
                           SCM_EOL, SCM_MODULE_OBARRAY (src));
   return SCM_UNSPECIFIED;
Index: lily/scm-hash.cc
diff --git a/lily/scm-hash.cc b/lily/scm-hash.cc
index  
0b1e0b4e2532a7cc481ba4a794270aa3caa44799..250f7170db875e9ebe65ff82dfb89bb426418fc0  
100644
--- a/lily/scm-hash.cc
+++ b/lily/scm-hash.cc
@@ -94,6 +94,6 @@ collect_handles (void * /* closure */,
 SCM
 Scheme_hash_table::to_alist () const
 {
-  return scm_internal_hash_fold ((scm_t_hash_fold_fn) &collect_handles,
+  return scm_internal_hash_fold (to_scm_t_hash_fold_fn (collect_handles),
                                  NULL, SCM_EOL, hash_tab ());
 }