Re: [PATCH 4/24] gdb: replace some so_list parameters to use references

Lancelot SIX
2023-10-19T15:20:34Z

>>> - void on_solib_loaded (so_list *so) override;
>>> - void on_solib_unloaded (so_list *so) override;
>>> + void on_solib_loaded (const so_list &so) override;
>>> + void on_solib_unloaded (const so_list &so) override;
>>
>> It is orthogonal to this change, but it would make sense for those
>> methods to be const as well.
>>
>> Doing this requires interp::interp_ui_out to be const as well (as done
>> in attached patch).
>
> Why do you think the interp object should be const (which is the effect
> of marking the methods const)? These methods are notifiers to let the
> interp know about certain events that occured. Whether they should
> modify the state of the interpreter or not is up to the particular
> implementation of the interpreter. Perhaps you're right, but I don't
> see it.
>

It is just that current interps (MI really) are just forwarding
notifications. The event does not change the interp's state.
Does it has to be const? No. But usually I tend to make things const
unless there is a reason not to. That's all.

That being said, making only those const would become quite
inconsistent with the rest of the interface. So in the end, that was
not a good idea!

> In your patch, you make interp::interp_ui_out const, but it still
> returns a non-const ui_out. So when outputting something, the
> interpreter itself is not modified, but the ui_out behind it is. I
> presume (sometimes) the ui_out needs to be non-const, since for things
> like pagination it clearly needs to keep a state. Design-wise, is it
> "correct" to make a const intepreter able to return a non-const ui_out?
>

I don't necessarily see that as an issue, the ui_out might need to
maintain its own state.

Anyway, since it is not really desirable to change this part of the
interface, I think this comment can be discarded.

>>> void on_about_to_proceed () override;
>>> void on_traceframe_changed (int tfnum, int tpnum) override;
>>> void on_tsv_created (const trace_state_variable *tsv) override;
>>> diff --git a/gdb/observable.h b/gdb/observable.h
>>> index acb05e9b535c..5ed6ca547ce0 100644
>>> --- a/gdb/observable.h
>>> +++ b/gdb/observable.h
>>> @@ -99,13 +99,12 @@ extern observable<inferior */* parent_inf */, inferior */* child_inf */,
>>> /* The shared library specified by SOLIB has been loaded. Note that
>>> when gdb calls this observer, the library's symbols probably
>>> haven't been loaded yet. */
>>> -extern observable<struct so_list */* solib */> solib_loaded;
>>> +extern observable<so_list &/* solib */> solib_loaded;
>>
>> I am wondering, is there a reason to make the solib parameter const for
>> solib_unloaded but not for solib_loaded?
>>
>> Changing it to const seems to still compile just fine. If down the line
>> an observer needs to modify the SO, the observer's signature can be
>> adjusted.
>
> Just pragmatic reasons. Do you build with --enable-targets=all?
>
> It's because bsd_uthread_solib_loaded calls solib_read_symbols to read
> in the symbols of the library it is looking for, and that requires a
> non-const so_list.
>

Ah, indeed, my current build tree does not have --enable-targets=all,
thanks. Having a user requiring a non-const ref is a good reason.

Thanks,
Lancelot.