[c++,05/12] guile: Constify gdbscm_with_guile return value

  Initially fixess:

/home/simark/src/binutils-gdb/gdb/guile/scm-disasm.c: In function ‘void* gdbscm_disasm_read_memory_worker(void*)’:
/home/simark/src/binutils-gdb/gdb/guile/scm-disasm.c:93:12: error: invalid conversion from ‘const void*’ to ‘void*’ [-fpermissive]
     return "seek error";

This makes const the whole path that leads to the return of
gdbscm_with_guile.

gdb/ChangeLog:

	* guile/guile-internal.h (gdbscm_with_guile): Constify function
	pointer return value and self return value.
	* guile/scm-safe-call.c (gdbscm_with_guile): Likewise.
	(struct c_data) <func>: Constify return value.
	(struct c_data) <result>: Constify.
	(scscm_eval_scheme_string): Constify return value.
	(gdbscm_safe_eval_string): Constify status variable.
	(scscm_source_scheme_script): Constify return value.
	(gdbscm_safe_source_script): Constify status variable.
	* guile/scm-disasm.c (gdbscm_disasm_read_memory_worker):
	Constify returrn value.
	(gdbscm_disasm_read_memory): Constify status variable.
i
---
 gdb/guile/guile-internal.h |  2 +-
 gdb/guile/scm-disasm.c     |  4 ++--
 gdb/guile/scm-safe-call.c  | 20 ++++++++++----------
 3 files changed, 13 insertions(+), 13 deletions(-)
  

Simon Marchi <simon.marchi@polymtl.ca> writes:
> Initially fixess:
>
> /home/simark/src/binutils-gdb/gdb/guile/scm-disasm.c: In function ‘void* gdbscm_disasm_read_memory_worker(void*)’:
> /home/simark/src/binutils-gdb/gdb/guile/scm-disasm.c:93:12: error: invalid conversion from ‘const void*’ to ‘void*’ [-fpermissive]
>      return "seek error";
>
> This makes const the whole path that leads to the return of
> gdbscm_with_guile.
>
> gdb/ChangeLog:
>
> 	* guile/guile-internal.h (gdbscm_with_guile): Constify function
> 	pointer return value and self return value.
> 	* guile/scm-safe-call.c (gdbscm_with_guile): Likewise.
> 	(struct c_data) <func>: Constify return value.
> 	(struct c_data) <result>: Constify.
> 	(scscm_eval_scheme_string): Constify return value.
> 	(gdbscm_safe_eval_string): Constify status variable.
> 	(scscm_source_scheme_script): Constify return value.
> 	(gdbscm_safe_source_script): Constify status variable.
> 	* guile/scm-disasm.c (gdbscm_disasm_read_memory_worker):
> 	Constify returrn value.
> 	(gdbscm_disasm_read_memory): Constify status variable.

Hi.

How about instead having gdbscm_with_guile return a const char *.
That should, for example, remove the need for any cast here in
gdbscm_safe_source_script (and presumably elsewhere):

  if (result != NULL)
    return xstrdup ((char *) result);

>
> diff --git a/gdb/guile/guile-internal.h b/gdb/guile/guile-internal.h
> index 017309a..0af01d2 100644
> --- a/gdb/guile/guile-internal.h
> +++ b/gdb/guile/guile-internal.h
> @@ -384,7 +384,7 @@ extern void gdbscm_memory_error (const char *subr, const char *msg, SCM args)
>  
>  /* scm-safe-call.c */
>  
> -extern void *gdbscm_with_guile (void *(*func) (void *), void *data);
> +extern const void *gdbscm_with_guile (const void *(*func) (void *), void *data);
>  
>  extern SCM gdbscm_call_guile (SCM (*func) (void *), void *data,
>  			      excp_matcher_func *ok_excps);
> diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
> index d1572c7..63889c1 100644
> --- a/gdb/guile/scm-disasm.c
> +++ b/gdb/guile/scm-disasm.c
> @@ -76,7 +76,7 @@ dascm_make_insn (CORE_ADDR pc, const char *assembly, int insn_len)
>     Scheme port.  Called via gdbscm_call_guile.
>     The result is a statically allocated error message or NULL if success.  */
>  
> -static void *
> +static const void *
>  gdbscm_disasm_read_memory_worker (void *datap)
>  {
>    struct gdbscm_disasm_read_data *data
> @@ -109,7 +109,7 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
>  			   struct disassemble_info *dinfo)
>  {
>    struct gdbscm_disasm_read_data data;
> -  void *status;
> +  const void *status;
>  
>    data.memaddr = memaddr;
>    data.myaddr = myaddr;
> diff --git a/gdb/guile/scm-safe-call.c b/gdb/guile/scm-safe-call.c
> index 62aec0f..85a2a5b 100644
> --- a/gdb/guile/scm-safe-call.c
> +++ b/gdb/guile/scm-safe-call.c
> @@ -28,10 +28,10 @@
>  
>  struct c_data
>  {
> -  void *(*func) (void *);
> +  const void *(*func) (void *);
>    void *data;
>    /* An error message or NULL for success.  */
> -  void *result;
> +  const void *result;
>  };
>  
>  /* Struct to marshall args through gdbscm_with_catch.  */
> @@ -167,8 +167,8 @@ gdbscm_with_catch (void *data)
>     The result if NULL if no exception occurred, otherwise it is a statically
>     allocated error message (caller must *not* free).  */
>  
> -void *
> -gdbscm_with_guile (void *(*func) (void *), void *data)
> +const void *
> +gdbscm_with_guile (const void *(*func) (void *), void *data)
>  {
>    struct c_data c_data;
>    struct with_catch_data catch_data;
> @@ -369,7 +369,7 @@ struct eval_scheme_string_data
>  /* Wrapper to eval a C string in the Guile interpreter.
>     This is passed to gdbscm_with_guile.  */
>  
> -static void *
> +static const void *
>  scscm_eval_scheme_string (void *datap)
>  {
>    struct eval_scheme_string_data *data
> @@ -398,12 +398,12 @@ char *
>  gdbscm_safe_eval_string (const char *string, int display_result)
>  {
>    struct eval_scheme_string_data data = { string, display_result };
> -  void *result;
> +  const void *result;
>  
>    result = gdbscm_with_guile (scscm_eval_scheme_string, (void *) &data);
>  
>    if (result != NULL)
> -    return xstrdup ((char *) result);
> +    return xstrdup ((const char *) result);
>    return NULL;
>  }
>  
> @@ -411,7 +411,7 @@ gdbscm_safe_eval_string (const char *string, int display_result)
>  
>  /* Helper function for gdbscm_safe_source_scheme_script.  */
>  
> -static void *
> +static const void *
>  scscm_source_scheme_script (void *data)
>  {
>    const char *filename = (const char *) data;
> @@ -439,7 +439,7 @@ gdbscm_safe_source_script (const char *filename)
>       by default.  This function is invoked by the "source" GDB command which
>       already has its own path search support.  */
>    char *abs_filename = NULL;
> -  void *result;
> +  const void *result;
>  
>    if (!IS_ABSOLUTE_PATH (filename))
>      {
> @@ -452,7 +452,7 @@ gdbscm_safe_source_script (const char *filename)
>  
>    xfree (abs_filename);
>    if (result != NULL)
> -    return xstrdup ((char *) result);
> +    return xstrdup ((const char *) result);
>    return NULL;
>  }
>
  

On 26 October 2015 at 01:22, Doug Evans <xdje42@gmail.com> wrote:
> Hi.
>
> How about instead having gdbscm_with_guile return a const char *.
> That should, for example, remove the need for any cast here in
> gdbscm_safe_source_script (and presumably elsewhere):
>
>   if (result != NULL)


Hmmm, looking more at the issue, I see that we happen to call
gdbscm_with_guile only with functions that return const char *.  But
it doesn't mean we should make gdbscm_with_guile return const char *
ncecessarily. It (and scm_with_guile) takes a void* and returns a
void* in order to be generic, just like in the pthread_create/join
API. The caller is responsible to cast the void* to the right type.
In C++, we could always get fancy and make it a templated function to
get type-safety.

So in retrospect, I think we should leave the original return type
(non-const void*) and just add the appropriate casts.

Does that make sense?
  

Simon Marchi <simon.marchi@polymtl.ca> writes:
> On 26 October 2015 at 01:22, Doug Evans <xdje42@gmail.com> wrote:
>> Hi.
>>
>> How about instead having gdbscm_with_guile return a const char *.
>> That should, for example, remove the need for any cast here in
>> gdbscm_safe_source_script (and presumably elsewhere):
>>
>>   if (result != NULL)
>
>
> Hmmm, looking more at the issue, I see that we happen to call
> gdbscm_with_guile only with functions that return const char *.  But
> it doesn't mean we should make gdbscm_with_guile return const char *
> ncecessarily. It (and scm_with_guile) takes a void* and returns a
> void* in order to be generic, just like in the pthread_create/join
> API. The caller is responsible to cast the void* to the right type.
> In C++, we could always get fancy and make it a templated function to
> get type-safety.
>
> So in retrospect, I think we should leave the original return type
> (non-const void*) and just add the appropriate casts.
>
> Does that make sense?

The function comment for gdbscm_with_guile says:

/* A wrapper around scm_with_guile that prints backtraces and exceptions
   according to "set guile print-stack".
   The result if NULL if no exception occurred, otherwise it is a statically
   allocated error message (caller must *not* free).  */

If we're going to return an error message,
why make it a void * and not a char * (const as appropriate)?

The lower level guile API uses a void * because it doesn't specify what
the result is. But in this use of it we do specify what the result is.