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

Zoran Zaric via Gdb-patches
2021-04-28T13:17:02Z

On 4/28/21 4:04 AM, Simon Marchi wrote:
> [CAUTION: External Email]
>
>> @@ -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
>

Agreed, I will change it in the next iteration.

Zoran