[RFC v2] Unnecessary argument METHOD to valops.c:find_oload_champ?

[RFC v2] Unnecessary argument METHOD to valops.c:find_oload_champ?

[Siva Chandra]

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

Realized I did not update function comment in v1.  Attached v2 which
includes a typo fix and updates the function comment.

v1 posting: https://sourceware.org/ml/gdb-patches/2014-02/msg00833.html

ChangeLog
2014-02-28  Siva Chandra Reddy  <sivachandra@google.com>

        * valops.c (find_oload_champ): Remove unneccesary argument METHOD
        (find_overload_match): Update call to find_oload_champ.
        (find_oload_champ_namespace_loop): Likewise

Thanks,
Siva Chandra

[-- Attachment #2: find_oload_champ_patch_v2.txt --]
[-- Type: text/plain, Size: 4185 bytes --]

diff --git a/gdb/valops.c b/gdb/valops.c
index 9d2218f..3822c87 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -69,7 +69,7 @@ int find_oload_champ_namespace_loop (struct value **, int,
 				     struct badness_vector **, int *,
 				     const int no_adl);
 
-static int find_oload_champ (struct value **, int, int, int,
+static int find_oload_champ (struct value **, int, int,
 			     struct fn_field *, struct symbol **,
 			     struct badness_vector **);
 
@@ -2469,9 +2469,9 @@ find_overload_match (struct value **args, int nargs,
       if (fns_ptr)
 	{
 	  gdb_assert (TYPE_DOMAIN_TYPE (fns_ptr[0].type) != NULL);
-	  method_oload_champ = find_oload_champ (args, nargs, method,
+	  method_oload_champ = find_oload_champ (args, nargs,
 	                                         num_fns, fns_ptr,
-	                                         oload_syms, &method_badness);
+	                                         NULL, &method_badness);
 
 	  method_match_quality =
 	      classify_oload_match (method_badness, nargs,
@@ -2784,7 +2784,7 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
   while (new_oload_syms[num_fns])
     ++num_fns;
 
-  new_oload_champ = find_oload_champ (args, nargs, 0, num_fns,
+  new_oload_champ = find_oload_champ (args, nargs, num_fns,
 				      NULL, new_oload_syms,
 				      &new_oload_champ_bv);
 
@@ -2823,20 +2823,22 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
 
 /* Look for a function to take NARGS args of ARGS.  Find
    the best match from among the overloaded methods or functions
-   (depending on METHOD) given by FNS_PTR or OLOAD_SYMS, respectively.
-   The number of methods/functions in the list is given by NUM_FNS.
+   given by FNS_PTR or OLOAD_SYMS, respectively.  One, and only one of
+   FNS_PTR and OLOAD_SYMS can be non-NULL.  The number of
+   methods/functions in the non-NULL list is given by NUM_FNS.
    Return the index of the best match; store an indication of the
    quality of the match in OLOAD_CHAMP_BV.
 
    It is the caller's responsibility to free *OLOAD_CHAMP_BV.  */
 
 static int
-find_oload_champ (struct value **args, int nargs, int method,
+find_oload_champ (struct value **args, int nargs,
 		  int num_fns, struct fn_field *fns_ptr,
 		  struct symbol **oload_syms,
 		  struct badness_vector **oload_champ_bv)
 {
   int ix;
+  int domain_count = (fns_ptr != NULL? 1 : 0) + (oload_syms != NULL ? 1 : 0);
   /* A measure of how good an overloaded instance is.  */
   struct badness_vector *bv;
   /* Index of best overloaded function.  */
@@ -2845,31 +2847,37 @@ find_oload_champ (struct value **args, int nargs, int method,
   int oload_ambiguous = 0;
   /* 0 => no ambiguity, 1 => two good funcs, 2 => incomparable funcs.  */
 
+  /* A champion can be found among methods alone, or among functions
+     alone, but not both.  */
+  gdb_assert (domain_count == 1);
+
   *oload_champ_bv = NULL;
 
   /* Consider each candidate in turn.  */
   for (ix = 0; ix < num_fns; ix++)
     {
       int jj;
-      int static_offset = oload_method_static (method, fns_ptr, ix);
+      int static_offset;
       int nparms;
       struct type **parm_types;
 
-      if (method)
+      if (fns_ptr != NULL)
 	{
 	  nparms = TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr, ix));
+	  static_offset = oload_method_static (1, fns_ptr, ix);
 	}
       else
 	{
 	  /* If it's not a method, this is the proper place.  */
 	  nparms = TYPE_NFIELDS (SYMBOL_TYPE (oload_syms[ix]));
+	  static_offset = 0;
 	}
 
       /* Prepare array of parameter types.  */
       parm_types = (struct type **) 
 	xmalloc (nparms * (sizeof (struct type *)));
       for (jj = 0; jj < nparms; jj++)
-	parm_types[jj] = (method
+	parm_types[jj] = (fns_ptr != NULL
 			  ? (TYPE_FN_FIELD_ARGS (fns_ptr, ix)[jj].type)
 			  : TYPE_FIELD_TYPE (SYMBOL_TYPE (oload_syms[ix]), 
 					     jj));
@@ -2907,7 +2915,7 @@ find_oload_champ (struct value **args, int nargs, int method,
       xfree (parm_types);
       if (overload_debug)
 	{
-	  if (method)
+	  if (fns_ptr)
 	    fprintf_filtered (gdb_stderr,
 			      "Overloaded method instance %s, # of parms %d\n",
 			      fns_ptr[ix].physname, nparms);

Re: [RFC v2] Unnecessary argument METHOD to valops.c:find_oload_champ?

[Doug Evans]

On Fri, Feb 28, 2014 at 6:33 AM, Siva Chandra <sivachandra@google.com> wrote:
> Realized I did not update function comment in v1.  Attached v2 which
> includes a typo fix and updates the function comment.
>
> v1 posting: https://sourceware.org/ml/gdb-patches/2014-02/msg00833.html
>
> ChangeLog
> 2014-02-28  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * valops.c (find_oload_champ): Remove unneccesary argument METHOD
>         (find_overload_match): Update call to find_oload_champ.
>         (find_oload_champ_namespace_loop): Likewise
>
> Thanks,
> Siva Chandra

Hi.

Ok with me, with one nit:

I'd collapse this into just a single assert:

   gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) == 1).

---
+  int domain_count = (fns_ptr != NULL? 1 : 0) + (oload_syms != NULL ? 1 : 0);
[...]
+  /* A champion can be found among methods alone, or among functions
+     alone, but not both.  */
+  gdb_assert (domain_count == 1);

Thanks!

Btw, I see room for more cleanups.

E.g., oload_method_static could use some TLC.
- it's passed an enum for the method arg where we assume non-zero
values are correct
- IWBN to delete its method arg too
- its result feels like it's intended to be true/false, but it's used
as an integer
  (rename function to oload_method_static_offset ?)

I wouldn't make them part of the current patch, and feel free to pass.
But since you're in the neighborhood ... :-)

Re: [RFC v2] Unnecessary argument METHOD to valops.c:find_oload_champ?

[Siva Chandra]

On Fri, Feb 28, 2014 at 12:33 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Feb 28, 2014 at 6:33 AM, Siva Chandra <sivachandra@google.com> wrote:
>> Realized I did not update function comment in v1.  Attached v2 which
>> includes a typo fix and updates the function comment.
>>
>> v1 posting: https://sourceware.org/ml/gdb-patches/2014-02/msg00833.html
>>
>> ChangeLog
>> 2014-02-28  Siva Chandra Reddy  <sivachandra@google.com>
>>
>>         * valops.c (find_oload_champ): Remove unneccesary argument METHOD
>>         (find_overload_match): Update call to find_oload_champ.
>>         (find_oload_champ_namespace_loop): Likewise
>>
>> Thanks,
>> Siva Chandra
>
> Hi.
>
> Ok with me, with one nit:
>
> I'd collapse this into just a single assert:
>
>    gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) == 1).
>
> ---
> +  int domain_count = (fns_ptr != NULL? 1 : 0) + (oload_syms != NULL ? 1 : 0);
> [...]
> +  /* A champion can be found among methods alone, or among functions
> +     alone, but not both.  */
> +  gdb_assert (domain_count == 1);
>
> Thanks!
>
> Btw, I see room for more cleanups.
>
> E.g., oload_method_static could use some TLC.
> - it's passed an enum for the method arg where we assume non-zero
> values are correct
> - IWBN to delete its method arg too
> - its result feels like it's intended to be true/false, but it's used
> as an integer
>   (rename function to oload_method_static_offset ?)
>
> I wouldn't make them part of the current patch, and feel free to pass.
> But since you're in the neighborhood ... :-)

Re: [RFC v2] Unnecessary argument METHOD to valops.c:find_oload_champ?

[Siva Chandra]

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

I apologize for the previous send; some magic gmail shortcut seems to
have sent it before I wanted to press the send button.

On Fri, Feb 28, 2014 at 12:33 PM, Doug Evans <dje@google.com> wrote:
>
> I'd collapse this into just a single assert:
>
>    gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) == 1).

Pushed the attached patch which addresses this.

> Btw, I see room for more cleanups.
>
> E.g., oload_method_static could use some TLC.
> - it's passed an enum for the method arg where we assume non-zero
> values are correct
> - IWBN to delete its method arg too
> - its result feels like it's intended to be true/false, but it's used
> as an integer
>   (rename function to oload_method_static_offset ?)
>
> I wouldn't make them part of the current patch, and feel free to pass.
> But since you're in the neighborhood ... :-)

Will take it up shortly.

[-- Attachment #2: find_oload_champ_patch_v3.txt --]
[-- Type: text/plain, Size: 5023 bytes --]

From 9cf953733af60dbcd554cd744c560637233449bb Mon Sep 17 00:00:00 2001
From: Siva Chandra <sivachandra@chromium.org>
Date: Thu, 27 Feb 2014 05:51:46 -0800
Subject: [PATCH] Remove the unnecesary argument METHOD to
 valops.c:find_oload_champ.

	* valops.c (find_oload_champ): Remove unneccesary argument METHOD.
	(find_overload_match): Update call to find_oload_champ.
	(find_oload_champ_namespace_loop): Likewise
---
 gdb/ChangeLog |  6 ++++++
 gdb/valops.c  | 29 ++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ef6cf7c..2c29950 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-02-28  Siva Chandra Reddy  <sivachandra@google.com>
+
+	* valops.c (find_oload_champ): Remove unneccesary argument METHOD.
+	(find_overload_match): Update call to find_oload_champ.
+	(find_oload_champ_namespace_loop): Likewise
+
 2014-02-28  Mark Kettenis  <kettenis@gnu.org>
 
 	* Makefile.in (ALLDEPFILES): Add sparc64obsd-nat.c.
diff --git a/gdb/valops.c b/gdb/valops.c
index 9d2218f..cf195a3 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -69,7 +69,7 @@ int find_oload_champ_namespace_loop (struct value **, int,
 				     struct badness_vector **, int *,
 				     const int no_adl);
 
-static int find_oload_champ (struct value **, int, int, int,
+static int find_oload_champ (struct value **, int, int,
 			     struct fn_field *, struct symbol **,
 			     struct badness_vector **);
 
@@ -2469,9 +2469,9 @@ find_overload_match (struct value **args, int nargs,
       if (fns_ptr)
 	{
 	  gdb_assert (TYPE_DOMAIN_TYPE (fns_ptr[0].type) != NULL);
-	  method_oload_champ = find_oload_champ (args, nargs, method,
+	  method_oload_champ = find_oload_champ (args, nargs,
 	                                         num_fns, fns_ptr,
-	                                         oload_syms, &method_badness);
+	                                         NULL, &method_badness);
 
 	  method_match_quality =
 	      classify_oload_match (method_badness, nargs,
@@ -2784,7 +2784,7 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
   while (new_oload_syms[num_fns])
     ++num_fns;
 
-  new_oload_champ = find_oload_champ (args, nargs, 0, num_fns,
+  new_oload_champ = find_oload_champ (args, nargs, num_fns,
 				      NULL, new_oload_syms,
 				      &new_oload_champ_bv);
 
@@ -2823,15 +2823,16 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
 
 /* Look for a function to take NARGS args of ARGS.  Find
    the best match from among the overloaded methods or functions
-   (depending on METHOD) given by FNS_PTR or OLOAD_SYMS, respectively.
-   The number of methods/functions in the list is given by NUM_FNS.
+   given by FNS_PTR or OLOAD_SYMS, respectively.  One, and only one of
+   FNS_PTR and OLOAD_SYMS can be non-NULL.  The number of
+   methods/functions in the non-NULL list is given by NUM_FNS.
    Return the index of the best match; store an indication of the
    quality of the match in OLOAD_CHAMP_BV.
 
    It is the caller's responsibility to free *OLOAD_CHAMP_BV.  */
 
 static int
-find_oload_champ (struct value **args, int nargs, int method,
+find_oload_champ (struct value **args, int nargs,
 		  int num_fns, struct fn_field *fns_ptr,
 		  struct symbol **oload_syms,
 		  struct badness_vector **oload_champ_bv)
@@ -2845,31 +2846,37 @@ find_oload_champ (struct value **args, int nargs, int method,
   int oload_ambiguous = 0;
   /* 0 => no ambiguity, 1 => two good funcs, 2 => incomparable funcs.  */
 
+  /* A champion can be found among methods alone, or among functions
+     alone, but not both.  */
+  gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) == 1);
+
   *oload_champ_bv = NULL;
 
   /* Consider each candidate in turn.  */
   for (ix = 0; ix < num_fns; ix++)
     {
       int jj;
-      int static_offset = oload_method_static (method, fns_ptr, ix);
+      int static_offset;
       int nparms;
       struct type **parm_types;
 
-      if (method)
+      if (fns_ptr != NULL)
 	{
 	  nparms = TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr, ix));
+	  static_offset = oload_method_static (1, fns_ptr, ix);
 	}
       else
 	{
 	  /* If it's not a method, this is the proper place.  */
 	  nparms = TYPE_NFIELDS (SYMBOL_TYPE (oload_syms[ix]));
+	  static_offset = 0;
 	}
 
       /* Prepare array of parameter types.  */
       parm_types = (struct type **) 
 	xmalloc (nparms * (sizeof (struct type *)));
       for (jj = 0; jj < nparms; jj++)
-	parm_types[jj] = (method
+	parm_types[jj] = (fns_ptr != NULL
 			  ? (TYPE_FN_FIELD_ARGS (fns_ptr, ix)[jj].type)
 			  : TYPE_FIELD_TYPE (SYMBOL_TYPE (oload_syms[ix]), 
 					     jj));
@@ -2907,7 +2914,7 @@ find_oload_champ (struct value **args, int nargs, int method,
       xfree (parm_types);
       if (overload_debug)
 	{
-	  if (method)
+	  if (fns_ptr)
 	    fprintf_filtered (gdb_stderr,
 			      "Overloaded method instance %s, # of parms %d\n",
 			      fns_ptr[ix].physname, nparms);
-- 
1.9.0.279.gdc9e3eb