Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes

[Simon Marchi <simark@simark.ca>]

On 2024-10-10 10:10, Guinevere Larsen wrote:
> On 10/4/24 2:21 AM, Simon Marchi wrote:
>> Some more things I thought about...
>>
>>>> +  /* Default frame sniffer.  Will always return that the unwinder
>>>> +     is able to unwind the frame.  */
>>>> +  virtual int sniffer (const frame_unwind *self,
>> One nit: I think this method should be called just "sniff".  When a
>> method is named after a thing, my brain adds an implicit "get_" in front
>> of it.  So to me, a method named "sniffer" would return some sniffer
>> object.  Here, I think it would make more sense to use an action verb,
>> because we want the unwinder to "sniff".
> This makes sense, and is a pretty trivial change. Fixed.
>>
>> Oh and I guess another naming nit, I think frame_unwind should be named
>> frame_unwinder.  A "thing" that unwinds stuff would be called an
>> "unwinder", not an "unwind".  I don't know how you feel about changing
>> that.
> 
> This is a reasonably big change, because several functions receive frame_unwind as a parameter.

I would expect it to be almost a 3 click change in any reasonably smart
IDE :).

> If we progress conversion from the legacy to the specific classes we're going to reduce the number of function that get it as a parameter and it will turn into a smaller, more manageable patch. I think makes sense for future improvement.

Ack.

>> And something else I just remembered, I am not really a fan of the
>> very abbreviated parameter names like this:
>>
>>    frame_unwind_trampoline (enum frame_type t, const struct frame_data *d,
>>                frame_prev_arch_ftype *pa)
>>      : frame_unwind ("trampoline", t, FRAME_UNWIND_GDB, d), prev_arch_p (pa)
>>    { }
>>
>> When using an editor that provides tooltips when writing a function
>> call, the abbreviated names in the tooltip are not very helpful.  I
>> would suggest using proper names.
> This makes sense. I really dislike having a function named the same as a type, but I will use "type", "data" and "prev_arch_func".

Sounds good.

Simon