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

Doug Evans
2013-11-21T01:45:12Z

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.