Re: [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations")

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

>  > gdb/ChangeLog:
>  >
>  >         * location.h (new_address_location): Add new parameters
>  >         "addr_string" and "addr_string_len".
>  >         (get_address_string_location): Add declaration.
>  >         * location.c (new_address_location): Add new parameters
>  >         "addr_string" and "addr_string_len".  If not NULL, store
>  >         a copy of the addr_string in the new location as well.
>  >         (get_address_string_location): New function.
>  >         (string_to_event_location): Update call to new_address_location.
>  >         * linespec.c (event_location_to_sals) <ADDRESS_LOCATION>:
>  >         Save the event location in the parser's state before
>  >         passing it to convert_address_location_to_sals.
>  >         * breakpoint.c (create_thread_event_breakpoint): Update call
>  >         to new_address_location.
>  >         (init_breakpoint_sal): Get the event location's string, if any,
>  >         and use it to update call to new_address_location.
>  >         * python/py-finishbreakpoint.c (bpfinishpy_init):
>  >         Update call to new_address_location.
>  >         * spu-tdep.c (spu_catch_start): Likewise.
>  >
>  >         * config/djgpp/fnchange.lst: Add entries for
>  >         gdb/testsuite/gdb.base/break-fun-addr1.c and
>  >         gdb/testsuite/gdb.base/break-fun-addr2.c.
>  >
>  > gdb/testsuite/ChangeLog:
>  >
>  >         * gdb.base/break-fun-addr.exp: New file.
>  >         * gdb.base/break-fun-addr1.c: New file.
>  >         * gdb.base/break-fun-addr2.c: New file.
>  >
>  > Tested on x86_64-linux.
>  > +const char *
>  > +get_address_string_location (const struct event_location *location)
>  > +{
>  > +  gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
>  > +  return EL_STRING (location);
>  > +}
>  > +
>  > +/* See description in location.h.  */
>  > +
> 
> Given that the argument must be an address location,
> "get_address_location_string" reads better to me.

I am not sure what my exact thinking was, at the time, and
whether it was flawed or not, but I chose the naming at the time
to be consistent with the other functions that looked similar in
they functionality. The ideal name for the function would probably
be something like "get_address_string_from_location", in other
words, what I meant is to return the "address string" (as opposed
to the address itself).

For now, I pushed the patch as is, but if you think the name
should be changed, here is a patch that does that. So, it's ready
to go in, if you give the signal.

What I would (counter-)propose, on the other hand, is that we
look at renaming the function to use "_from_location". Or, maybe
have the functions start with "location_" (Eg:
location_get_address_string, which actually might be closer
to our typical namespace strategy).

-- 
Joel

[-- Attachment #2: 0001-rename-get_address_string_location-into-get_address_.patch --]
[-- Type: text/x-diff, Size: 2533 bytes --]

From 493d020497093787700e1a055a25f8d6471ea416 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 21 Jan 2016 14:26:33 +0400
Subject: [PATCH] rename get_address_string_location into
 get_address_location_string

The new function name that sounds more correct.

gdb/ChangeLog:

        * location.h, linespec.c, location.c (get_address_location_string):
        Renames get_address_string_location.
---
 gdb/ChangeLog  | 5 +++++
 gdb/linespec.c | 2 +-
 gdb/location.c | 2 +-
 gdb/location.h | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a377a32..598ac9d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-01-21  Joel Brobecker  <brobecker@adacore.com>
 
+	* location.h, linespec.c, location.c (get_address_location_string):
+	Renames get_address_string_location.
+
+2016-01-21  Joel Brobecker  <brobecker@adacore.com>
+
 	* location.h (new_address_location): Add new parameters
 	"addr_string" and "addr_string_len".
 	(get_address_string_location): Add declaration.
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 2360cc1..9c8a27a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2513,7 +2513,7 @@ event_location_to_sals (linespec_parser *parser,
 
     case ADDRESS_LOCATION:
       {
-	const char *addr_string = get_address_string_location (location);
+	const char *addr_string = get_address_location_string (location);
 	CORE_ADDR addr = get_address_location (location);
 
 	if (addr_string != NULL)
diff --git a/gdb/location.c b/gdb/location.c
index e43ebf1..88ae61d 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -138,7 +138,7 @@ get_address_location (const struct event_location *location)
 /* See description in location.h.  */
 
 const char *
-get_address_string_location (const struct event_location *location)
+get_address_location_string (const struct event_location *location)
 {
   gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
   return EL_STRING (location);
diff --git a/gdb/location.h b/gdb/location.h
index b2cf45e..2b04def 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -145,7 +145,7 @@ extern CORE_ADDR
    of the given event_location (which must be of type ADDRESS_LOCATION).  */
 
 extern const char *
-  get_address_string_location (const struct event_location *location);
+  get_address_location_string (const struct event_location *location);
 
 /* Create a new probe location.  The return result is malloc'd
    and should be freed with delete_event_location.  */
-- 
2.5.0