Re: [PATCH 17/43] Add as_lval argument to expression evaluator

Simon Marchi via Gdb-patches
2021-04-28T03:04:23Z

> @@ -929,6 +930,11 @@ dwarf_expr_context::fetch_result (struct type *type,
> }
> else
> {
> + /* If AS_LVAL is false, means that the implicit conversion
> + from a location description to value is expected. */
> + if (as_lval == false)

For booleans: !as_lval

> diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
> index d1374068732..3f519e81849 100644
> --- a/gdb/dwarf2/expr.h
> +++ b/gdb/dwarf2/expr.h
> @@ -130,11 +130,13 @@ struct dwarf_expr_context
> void push_address (CORE_ADDR value, bool in_stack_memory);
>
> /* Evaluate the expression at ADDR (LEN bytes long) in a given PER_CU
> - FRAME context. Where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET describe
> - expected struct value representation of the evaluation result.
> - The ADDR_INFO property can be specified to override the range of
> - memory addresses with the passed in buffer. */
> - struct value *evaluate (const gdb_byte *addr, size_t len,
> + FRAME context. AS_LVAL defines if the returned struct value is
> + expected to be a value or a location description. Where TYPE,
> + SUBOBJ_TYPE and SUBOBJ_OFFSET describe expected struct value
> + representation of the evaluation result. The ADDR_INFO property
> + can be specified to override the range of memory addresses with
> + the passed in buffer. */

Especially when the comment starts getting long and there are many
parameters, I think it helps readability to space it out a bit. Start
with one sentence by itself that gives the high level description of
what the method does. Then one paragraph per parameter (or group of
parameters, if some are logically connected). So, something like:

/* Evaluate the expression at ADDR (LEN bytes long) in a given PER_CU
and FRAME context.

AS_LVAL defines if the returned struct value is expected to be a
value or a location description.

TYPE, SUBOBJ_TYPE, SUBOBJ_OFFSET describe the expected struct value
representation of the evaluation result.

...

It would also be good to clarify for AS_LVAL which of true and false
mean "a value" and "a location description". It's not clear to me.

Simon