Re: [PATCH 1/3] Renaming in target-dcache.c

[Doug Evans <dje at google dot com>]

On Wed, Nov 20, 2013 at 5:16 PM, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> This patch does some renamings on "stack-cache" related functions and
> variables.
>
> In the review to "code cache" series v2, we have some discussions on the
> name of predicate function 'stack_cache_enabled', and have some options,
>
>  1 keep it unchanged, as it is already a predicate clearly,
>  2 rename it to stack_cache_enabled_p,
>  3 rename it to enable_stack_cache_p,
>
> I choose #2, because 'stack_cache_enabled' is a predicate, but
> it's better to add "_p" suffix to stress this.  There are some other
> similar patterns used in GDB source, such as unop_user_defined_p
> and agent_loaded_p.
>
> Then, I have to rename variable stack_cache_enabled_p to something
> else.  The option is "stack-cache", so I'd like to name the variable
> associated with this command as "stack_cache".  Similarly, the commands
> associated with this command should be renamed to "set_stack_cache"
> and "show_stack_cache" respectively.

I like the name "stack_cache_optimization_enabled".
I think it could help avoid future confusion.
But then the predicate should probably be stack_cache_optimization_enabled_p.
I don't have a problem with it, sometimes long and *really* clear
names are just what's called for.
I can imagine others wanting to keep it at stack_cache_enabled_p,
thus I could settle for stack_cache_enabled for the variable name.
"stack_cache" feels too nondescript.

[I think(!) we *could* also change the option name to
stack-cache-optimization, but I don't see as much a need to at the
moment.
I wouldn't do it for this patch series, just mentioning it for reference sake.]

>
> gdb:
>
> 2013-11-21  Yao Qi  <yao@codesourcery.com>
>
>         * target-dcache.c (stack_cache_enabled_p_1): Rename to ...
>         (stack_cache_1): ... this.  New variable.
>         (stack_cache_enabled_p): Rename to ...
>         (stack_cache): ... this.  New variable.
>         (set_stack_cache_enabled_p): Rename to ...
>         (set_stack_cache): ... this.  Update caller.
>         (show_stack_cache_enabled_p): Rename to ...
>         (show_stack_cache): ... this.  Update caller.
>         (stack_cache_enabled): Rename to ...
>         (stack_cache_enabled_p): ... this.  Update caller.
>         (_initialize_target_dcache): Replace "data cache" with
>         "target memory cache".
>         * target-dcache.h (stack_cache_enabled): Remove declaration.
>         (stack_cache_enabled_p): Add declaration.
> ---
>  gdb/target-dcache.c |   29 ++++++++++++++---------------
>  gdb/target-dcache.h |    2 +-
>  gdb/target.c        |    5 ++---
>  3 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
> index 76160ef..26f6f5d 100644
> --- a/gdb/target-dcache.c
> +++ b/gdb/target-dcache.c
> @@ -89,11 +89,11 @@ target_dcache_get_or_init (void)
>  }
>
>  /* The option sets this.  */
> -static int stack_cache_enabled_p_1 = 1;
> -/* And set_stack_cache_enabled_p updates this.
> +static int stack_cache_1 = 1;
> +/* And set_stack_cache updates this.
>     The reason for the separation is so that we don't flush the cache for
>     on->on transitions.  */
> -static int stack_cache_enabled_p = 1;
> +static int stack_cache = 1;
>
>  /* This is called *after* the stack-cache has been set.
>     Flush the cache for off->on and on->off transitions.
> @@ -101,18 +101,17 @@ static int stack_cache_enabled_p = 1;
>     except cleanliness.  */
>
>  static void
> -set_stack_cache_enabled_p (char *args, int from_tty,
> -                          struct cmd_list_element *c)
> +set_stack_cache (char *args, int from_tty, struct cmd_list_element *c)
>  {
> -  if (stack_cache_enabled_p != stack_cache_enabled_p_1)
> +  if (stack_cache != stack_cache_1)
>      target_dcache_invalidate ();
>
> -  stack_cache_enabled_p = stack_cache_enabled_p_1;
> +  stack_cache = stack_cache_1;
>  }
>
>  static void
> -show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
> -                           struct cmd_list_element *c, const char *value)
> +show_stack_cache (struct ui_file *file, int from_tty,
> +                 struct cmd_list_element *c, const char *value)
>  {
>    fprintf_filtered (file, _("Cache use for stack accesses is %s.\n"), value);
>  }
> @@ -120,9 +119,9 @@ show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
>  /* Return true if "stack cache" is enabled, otherwise, return false.  */
>
>  int
> -stack_cache_enabled (void)
> +stack_cache_enabled_p (void)
>  {
> -  return stack_cache_enabled_p;
> +  return stack_cache;
>  }
>
>  /* -Wmissing-prototypes */
> @@ -132,14 +131,14 @@ void
>  _initialize_target_dcache (void)
>  {
>    add_setshow_boolean_cmd ("stack-cache", class_support,
> -                          &stack_cache_enabled_p_1, _("\
> +                          &stack_cache_1, _("\
>  Set cache use for stack access."), _("\
>  Show cache use for stack access."), _("\
> -When on, use the data cache for all stack access, regardless of any\n\
> +When on, use the target memory cache for all stack access, regardless of any\n\
>  configured memory regions.  This improves remote performance significantly.\n\
>  By default, caching for stack access is on."),
> -                          set_stack_cache_enabled_p,
> -                          show_stack_cache_enabled_p,
> +                          set_stack_cache,
> +                          show_stack_cache,
>                            &setlist, &showlist);
>
>    target_dcache_aspace_key
> diff --git a/gdb/target-dcache.h b/gdb/target-dcache.h
> index a5e1556..3200dd9 100644
> --- a/gdb/target-dcache.h
> +++ b/gdb/target-dcache.h
> @@ -28,6 +28,6 @@ extern DCACHE *target_dcache_get_or_init (void);
>
>  extern int target_dcache_init_p (void);
>
> -extern int stack_cache_enabled (void);
> +extern int stack_cache_enabled_p (void);
>
>  #endif /* TARGET_DCACHE_H */
> diff --git a/gdb/target.c b/gdb/target.c
> index 5001643..29f06b6 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1547,7 +1547,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
>          the collected memory range fails.  */
>        && get_traceframe_number () == -1
>        && (region->attrib.cache
> -         || (stack_cache_enabled () && object == TARGET_OBJECT_STACK_MEMORY)))
> +         || (stack_cache_enabled_p () && object == TARGET_OBJECT_STACK_MEMORY)))
>      {
>        DCACHE *dcache = target_dcache_get_or_init ();
>
> @@ -1600,8 +1600,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
>        && writebuf != NULL
>        && target_dcache_init_p ()
>        && !region->attrib.cache
> -      && stack_cache_enabled ()
> -      && object != TARGET_OBJECT_STACK_MEMORY)
> +      && stack_cache_enabled_p () && object != TARGET_OBJECT_STACK_MEMORY)
>      {
>        DCACHE *dcache = target_dcache_get ();

I'd prefer to keep each && on a line by itself.

Ok with those changes.