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

Simon Marchi
2024-10-10T16:28:42Z

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