Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

On 2023-01-20 20:59, Tom Tromey wrote:
>>>>>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> v2 -> v3:
>> Addressed comments from Tom in
>> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html
> 
> Sorry, I'm afraid you missed a few.
> 
>> +struct dwarf2_frame_fn_data
>> +{
>> +  /* The cookie to identify the custom function data by.  */
>> +  fn_prev_register cookie;
> 
> The previous review mentioned changing the type of this, but honestly I
> don't really care about that one, this is as good as anything now that
> it's documented.
> 
>> +/* See frame.h.  */
>> +
>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
>> +				fn_prev_register cookie)
> 
> gdb style puts the function name at the start of the line.
> There are many examples in the source.
> 
>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>> +				     void **this_cache,
>> +				     fn_prev_register cookie,
>> +				     unsigned long size)
> 
> Here too.
> 
>> +{
>> +  struct dwarf2_frame_fn_data *fn_data = nullptr;
>> +  struct dwarf2_frame_cache *cache
>> +    = dwarf2_frame_cache (this_frame, this_cache);
>> +
>> +  /* First try to find an existing object.  */
>> +  void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, cookie);
>> +  if (data)
>> +    return data;
> 
> It seems to me that there is no need to do this check.
> IMO it's fine to just assert that it isn't found.
> 
> thanks,
> Tom

Thanks for the review.

I've corrected the points noted above. Still not sure why the 
check_GNU_style.py script in GCC contrib does not complain about this..

Anyway, is it okay for trunk with these changes or should I send a v4?

Kind regards,
Torbjörn