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

[Lancelot SIX <Lancelot.Six@amd.com>]

>>> -  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.