Re: [PATCH 2/6] invoke_xmethod & array_view

[Simon Marchi <simon.marchi@ericsson.com>]

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"

> 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)


> 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?

> +
> +  /* 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.

> 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

> @@ -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.

Simon