This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/6] invoke_xmethod & array_view
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