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

[Zoran Zaric via Gdb-patches <gdb-patches@sourceware.org>]

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