From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27844 invoked by alias); 21 Nov 2013 01:45:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27833 invoked by uid 89); 21 Nov 2013 01:45:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mail-vb0-f49.google.com Received: from Unknown (HELO mail-vb0-f49.google.com) (209.85.212.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 21 Nov 2013 01:45:11 +0000 Received: by mail-vb0-f49.google.com with SMTP id o19so7310479vbm.36 for ; Wed, 20 Nov 2013 17:45:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=z/T3yCWvg0lSKlH3MUW+wdMlzTY34cWGGoWgjRkxOuM=; b=PffuK49g0Ua5P+CiMGaSmPOQ9x3oqCwoNYQLYUZcZqtb+yoPnLwa3brgsc3w7T2jAr UPSMweyUowcFqyqwZYjX8iu0vzjwlw2/9mCa8NqzSfGydRGB4YkazODaEzm4IeFPx/62 56ofIKDlgPKYUsxy+t3mAJZ+fixuovD4FPv/o1ZwpgQiMMRfbNi+P2ioTf4BQIAN1cPc sKQrFFfRMezGqaAdMbFGeOr2/Y3l4ncS3MEOji/e9VxxDZy0Qd7toQU3+8gOTlFHn23c rnnTNqZqJmLxPIsOWCW0kT+L/mbFs2x6X/An7RoFWjclslq7BHtqoasCHdL9TWNPY1z4 zpmQ== X-Gm-Message-State: ALoCoQmqCFPe/Wk+twhoxzDc+qIbGSewddByV0ZCi4ihXKXVl716g22d6KENHNTqP7xI+VACh2E6cg48rpDU4b6xg37QEfGFyoWVUL3RZU8hmpji3z3+CG9OEVDaE/YHiV7mh6TlJY7Nps5tYVFVYDlyTBRmFqDKwymOaY9dU7aku+DJGQfhLzofKW26B3Shwwa9wDbitiD4rTiDbL/BWPcuVR96oeAyNQ== MIME-Version: 1.0 X-Received: by 10.58.161.231 with SMTP id xv7mr3254107veb.2.1384998302948; Wed, 20 Nov 2013 17:45:02 -0800 (PST) Received: by 10.52.163.52 with HTTP; Wed, 20 Nov 2013 17:45:02 -0800 (PST) In-Reply-To: <1384996594-20865-2-git-send-email-yao@codesourcery.com> References: <1384996594-20865-1-git-send-email-yao@codesourcery.com> <1384996594-20865-2-git-send-email-yao@codesourcery.com> Date: Thu, 21 Nov 2013 01:47:00 -0000 Message-ID: Subject: Re: [PATCH 1/3] Renaming in target-dcache.c From: Doug Evans To: Yao Qi Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00617.txt.bz2 On Wed, Nov 20, 2013 at 5:16 PM, Yao Qi 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 > > * 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.