[2/6] invoke_xmethod & array_view

From: Pedro Alves <palves@redhat.com>

  On 10/17/2018 05:21 PM, Simon Marchi wrote:
> On 2018-10-15 11:11 a.m., Pedro Alves wrote:
>> This replaces more pointer+length with gdb::array_view.  This time,
>> around invoke_xmethod, and then propagating the fallout around, which
>> inevitably leads to the overload resolution code.
>>
>> There are several places in the code that want to grab a slice of an
>> array, by advanting the array pointer, and decreasing the length
> 
> "advanting"

Fixed.

> 
>> pointer.  This patch introduces a pair of new
>> gdb::array_view::slice(...) methods to make that convenient and clear.
>> Unit test included.
> 
> Cool!
> 
> Just some minor comments.
> 
> I noticed some lines > 80 columns:
> 
> $ git show | grep ^\+ | tr '\t' '        ' | egrep '.{81}'
> +      return call_xmethod (argvec[0], gdb::make_array_view (argvec + 1, nargs));
> +value_user_defined_op (struct value **argp, gdb::array_view<value *> args, char *name,
> +      && classify_oload_match (new_oload_champ_bv, args.size (), 0) == STANDARD)
> 

Fixed the middle one, thanks.  The others are actually at 80 cols.

> 
>> diff --git a/gdb/common/array-view.h b/gdb/common/array-view.h
>> index 45d1a4720e..679d2e95c7 100644
>> --- a/gdb/common/array-view.h
>> +++ b/gdb/common/array-view.h
>> @@ -169,6 +169,17 @@ public:
>>    constexpr size_type size () const noexcept { return m_size; }
>>    constexpr bool empty () const noexcept { return m_size == 0; }
>>  
>> +  /* Slice an array view.  */
>> +
>> +  /* Return a new array view over SIZE elements starting at START.  */
>> +  constexpr array_view<T> slice (size_type start, size_t size) const noexcept
>> +  { return {m_array + start, size}; }
> 
> I'm sure there's a logic for using size_type for one parameter and size_t for the other
> (instead of size_type for both), but what is it?

There's no logic, just some copy&pasting&editing, I guess.

> 
>> +
>> +  /* Return a new array view over all the elements after START,
>> +     inclusive.  */
>> +  constexpr array_view<T> slice (size_type start) const noexcept
>> +  { return {m_array + start, size () - start}; }
> 
> It would perhaps be good to have some asserts (that are only there in development
> mode, maybe) to make sure we don't do stupid things, like take a slice
> past the end of the array, things like that.  A bit like those asserts enabled by
> __GLIBCXX_DEBUG.

Doing it in development mode might be useful.  In that case, it would
better be done to e.g., operator[] too, I think.  Also, not sure we
already have a development vs release preprocessor macro?
Thus I'd rather see it in a separate patch.

> 
>> diff --git a/gdb/extension.h b/gdb/extension.h
>> index 0c8c4ee934..4716d6f360 100644
>> --- a/gdb/extension.h
>> +++ b/gdb/extension.h
>> @@ -22,6 +22,7 @@
>>  
>>  #include "mi/mi-cmds.h" /* For PRINT_NO_VALUES, etc.  */
>>  #include "common/vec.h"
>> +#include "common/array-view.h"
>>  
>>  struct breakpoint;
>>  struct command_line;
>> @@ -186,38 +187,35 @@ struct xmethod_worker
>>    virtual ~xmethod_worker () = default;
>>  
>>    /* Invoke the xmethod encapsulated in this worker and return the result.
>> -     The method is invoked on OBJ with arguments in the ARGS array.  NARGS is
>> -     the length of the this array.  */
>> +     The method is invoked on OBJ with arguments in the ARGS array.  */
>>  
>> -  virtual value *invoke (value *obj, value **args, int nargs) = 0;
>> +  virtual value *invoke (value *obj, gdb::array_view<value *> args) = 0;
>>  
>>    /* Return the arg types of the xmethod encapsulated in this worker.
>> -     An array of arg types is returned.  The length of the array is returned in
>> -     NARGS.  The type of the 'this' object is returned as the first element of
>> -     array.  */
>> +     The type of the 'this' object is returned as the first element of
>> +     the vector.  */
>>  
>> -  type **get_arg_types (int *nargs);
>> +  std::vector<type *> get_arg_types ();
>>  
>>    /* Return the type of the result of the xmethod encapsulated in this worker.
>> -     OBJECT, ARGS, NARGS are the same as for invoke.  */
>> +     OBJECT and ARGS are the same as for invoke.  */
>>  
>> -  type *get_result_type (value *object, value **args, int nargs);
>> +  type *get_result_type (value *object, gdb::array_view<value *> args);
>>  
>>  private:
>>  
>> -  /* Return the types of the arguments the method takes.  The number of
>> -     arguments is returned in NARGS, and their types are returned in the array
>> -     ARGTYPES.  */
>> +  /* Return the types of the arguments the method takes.  The types
>> +     are returned in TYPE_ARGS, one per argument.  */
>>  
>>    virtual enum ext_lang_rc do_get_arg_types
>> -    (int *nargs, struct type ***arg_types) = 0;
>> +    (std::vector<type *> &type_args) = 0;
> 
> Could you change this to be a pointer to the std::vector?
> 
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
> 

I changed it, though I'm not a big fan of that rule.  

As I wrote at <https://gcc.gnu.org/ml/gcc/2018-07/msg00190.html>, I used to
prefer avoiding non-const reference output parameters too back in my
earlier C++ days, but I no longer do so much nowadays.

The main point of avoiding non-const reference parameters is that
supposedly they make function callers unclearer, with no
visual indication that the function modified the argument.  The
idea is that "&foo" instead of "foo" is that visual distinction.
But, that point can easily become moot because the visual distinction
can be easily lost with pointers too:

 // the usual argument is that using pointers for output parameters shows
 // clearly that bar is going to be modified:
 function (foo, &bar);

 // but well, we often works with pointers, and if "bar" is a pointer,
 // we don't get any visual clue anyway either:
 function (foo, bar);

 // which suggests that what really helps is seeing the output
 // variable nearby, suggesting to define it right before the
 // function call that fills it in, and I would go as far
 // as saying that clearer symbol names help even more.  For e.g.:
 B bar_out;
 fill_bar (foo, bar_out);

I think that symbol and function naming above is much more important
than "&bar" vs "bar".  A function called "fill_bar" is clearly going
to write to its "bar_out" argument.

Also, a pointer can be null, while a reference can not.  So
a reference parameter automatically implies that you can't pass
a NULL pointer to the function (which makes the function's
implementation a little bit clearer too), while with a pointer parameter
you have to document that, and maybe assert it.  With a reference, the
compiler is free to optimize accordingly (assume non-null), while with
a pointer, you have to use gcc's attribute nonnull if you want that,
which no one does.

Also, for std::vector parameters in particular, passing by pointer
leads to uglier code in the function's implementation, like e.g.,

 (*vec)[idx] = 0;

instead of:

 vec[idx] = 0;

We end up with a few instances like that in the series, though admittedly
not that many, otherwise I think I'd add something like:

  auto &vec = *vec_param;

at the top of the function and then use vec throughout.

So in sum, I nowadays tend to look at reference vs parameter more from
the "pointer: optional, can be NULL", vs "reference: non-optional" angle.
Though, given GDB's history, we definetely use pointers pervasively
in function parameters even when they're not supposed to be optional,
that's for sure.

Maybe we could come up with some middle ground rule, like always
putting in-out and output parameters last, and stressing to use
clear symbol names.

Anyway, I don't want to block on that discussion (my 1 month round trip
time not helping! :-D), so I did the change.

>> @@ -312,19 +312,19 @@ value_user_defined_cpp_op (struct value **args, int nargs, char *oper,
>>     function, otherwise return NULL.  */
>>  
>>  static struct value *
>> -value_user_defined_op (struct value **argp, struct value **args, char *name,
>> -                       int *static_memfuncp, int nargs, enum noside noside)
>> +value_user_defined_op (struct value **argp, gdb::array_view<value *> args, char *name,
>> +		       int *static_memfuncp, enum noside noside)
>>  {
>>    struct value *result = NULL;
>>  
>>    if (current_language->la_language == language_cplus)
>>      {
>> -      result = value_user_defined_cpp_op (args, nargs, name, static_memfuncp,
>> +      result = value_user_defined_cpp_op (args, name, static_memfuncp,
>>  					  noside);
>>      }
> 
> Maybe remove the extra braces, while touching this.

This is from <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Whitespaces>:

 "Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements: "

Here's what I merged.

From 6b1747cd135ff9859fceb6043179b1ef94363996 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 21 Nov 2018 11:55:12 +0000
Subject: [PATCH 2/6] invoke_xmethod & array_view

This replaces more pointer+length with gdb::array_view.  This time,
around invoke_xmethod, and then propagating the fallout around, which
inevitably leaks to the overload resolution code.

There are several places in the code that want to grab a slice of an
array, by advancing the array pointer, and decreasing the length
pointer.  This patch introduces a pair of new
gdb::array_view::slice(...) methods to make that convenient and clear.
Unit test included.

gdb/ChangeLog:
2018-11-21  Pedro Alves  <palves@redhat.com>

	* common/array-view.h (array_view::splice(size_type, size_t)): New.
	(array_view::splice(size_type)): New.
	* eval.c (eval_call, evaluate_funcall): Adjust to use array_view.
	* extension.c (xmethod_worker::get_arg_types): Adjust to return an
	std::vector.
	(xmethod_worker::get_result_type): Adjust to use gdb::array_view.
	* extension.h: Include "common/array-view.h".
	(xmethod_worker::invoke): Adjust to use gdb::array_view.
	(xmethod_worker::get_arg_types): Adjust to return an std::vector.
	(xmethod_worker::get_result_type): Adjust to use gdb::array_view.
	(xmethod_worker::do_get_arg_types): Adjust to use std::vector.
	(xmethod_worker::do_get_result_type): Adjust to use
	gdb::array_view.
	* gdbtypes.c (rank_function): Adjust to use gdb::array_view.
	* gdbtypes.h: Include "common/array-view.h".
	(rank_function): Adjust to use gdb::array_view.
	* python/py-xmethods.c (python_xmethod_worker::invoke)
	(python_xmethod_worker::do_get_arg_types)
	(python_xmethod_worker::do_get_result_type)
	(python_xmethod_worker::invoke): Adjust to new interfaces.
	* valarith.c (value_user_defined_cpp_op, value_user_defined_op)
	(value_x_binop, value_x_unop): Adjust to use gdb::array_view.
	* valops.c (find_overload_match, find_oload_champ_namespace)
	(find_oload_champ_namespace_loop, find_oload_champ): Adjust to use
	gdb:array_view and the new xmethod_worker interfaces.
	* value.c (result_type_of_xmethod, call_xmethod): Adjust to use
	gdb::array_view.
	* value.h (find_overload_match, result_type_of_xmethod)
	(call_xmethod): Adjust to use gdb::array_view.
	* unittests/array-view-selftests.c: Add slicing tests.
---
 gdb/ChangeLog                        |  33 +++++++++++
 gdb/common/array-view.h              |  11 ++++
 gdb/eval.c                           |  14 +++--
 gdb/extension.c                      |  12 ++--
 gdb/extension.h                      |  32 +++++------
 gdb/gdbtypes.c                       |  21 ++++---
 gdb/gdbtypes.h                       |   5 +-
 gdb/python/py-xmethods.c             |  40 ++++++--------
 gdb/unittests/array-view-selftests.c |  22 ++++++++
 gdb/valarith.c                       |  50 ++++++++---------
 gdb/valops.c                         | 104 +++++++++++++++++------------------
 gdb/value.c                          |  13 ++---
 gdb/value.h                          |   6 +-
 13 files changed, 212 insertions(+), 151 deletions(-)

[2/6] invoke_xmethod & array_view

Commit Message

Comments

Patch