Hi, David,
> On Oct 30, 2024, at 14:54, David Malcolm <dmalc...@redhat.com> wrote:
>
> On Wed, 2024-10-30 at 15:53 +0000, Qing Zhao wrote:
>>
>>
>>> On Oct 30, 2024, at 10:48, David Malcolm <dmalc...@redhat.com>
>>> wrote:
>>>
>>> On Wed, 2024-10-30 at 14:34 +0000, Sam James wrote:
>>>> Qing Zhao <qing.z...@oracle.com> writes:
>>>>
>>>>> Control this with a new option -fdiagnostics-details.
>>>>>
>
> [...]
>
>>
>> I have a question on the changes to the “warning_at”: (there are a
>> lot of such changes for -Warray-bounds and -Wstringop-**)
>>
>> - warned = warning_at (location, OPT_Warray_bounds_,
>> + {
>> + rich_location *richloc
>> + = build_rich_location_with_diagnostic_path (location,
>> stmt);
>> + warned = warning_at (richloc, OPT_Warray_bounds_,
>>
>> The above is the current change.
>>
>> My concern with this change is:
>> even when -fdiagnostics_details is NOT on, the rich_location is
>> created.
>
> A rich_location instance is always constructed when emitting
> diagnostics; warning_at with a location_t simply makes a rich_location
> on the stack.
Okay, I see. Thanks for the explanation.
>>
>> How much is the additional overhead when using “rich_location *”
>> other than “location_t” as the 1st argument of warning_at?
>
> The warning_at overload taking a rich_location * takes a borrowed
> pointer to a rich_location; it doesn't take ownership. Hence, as
> written, the patch has a memory leak: every call to
> build_rich_location_with_diagnostic_path is using "new" to make a new
> rich_location instance on the heap, and they aren't being deleted.
Oops, good catch!
>
>>
>> Should I control the creation of “rich_location" with the flag
>> “flag_diagnostics_details” (Similar as I control the creation of
>> “move_history” data structure with the flag
>> “flag_diagnostics_details”?
>>
>> If so, how should I do it? Do you have a suggestion on a clean and
>> simply coding here (Sorry for the stupid question on this)
>
> You can probably do all of this on the stack; make a new rich_location
> subclass, with something like:
>
> class rich_location_with_details : public gcc_rich_location
> {
> public:
> rich_location_with_details (location_t location, gimple *stmt);
>
> private:
> class deferred_move_history_path {
> public:
> deferred_move_history_path (location_t location, gimple *stmt)
> : m_location (location), m_stmt (stmt)
> {
> }
>
> std::unique_ptr<diagnostic_path>
> make_path () const final override;
> /* TODO: you'll need to implement this; it will be called on
> demand if a diagnostic is acutally emitted for this
> rich_location. */
What do you mean by “it will be called on demand if a diagnostic is actually
emitted”?
Do I need to do anything special in the code to call this “make_path”?
>
> location_t m_location;
> gimple *m_stmt;
> } m_deferred_move_history_path;
> };
>
> rich_location_with_details::
> rich_location_with_details (location_t location, gimple *stmt)
> : gcc_rich_location (location),
> m_deferred_move_history_path (location, stmt)
> {
> set_path (&m_deferred_move_history_path);
> }
>
> using class deferred_diagnostic_path from the attached patch (caveat: I
> haven't tried bootstrapping it yet).
So, I also need to add the new class “deferred_diangostic_path” ?
>
> With that support subclass, you should be able to do something like
> this to make them on the stack:
>
> rich_location_with_details richloc (location, stmt);
> warned = warning_at (&richloc, OPT_Warray_bounds_,
> "array subscript %E is outside array"
> " bounds of %qT", low_sub_org, artype);
>
> and no work will be done for path creation unless and until a
> diagnostic is actually emitted for richloc - the richloc ctor will just
> initialize the vtable and some location_t/gimple * fields, which ought
> to be very cheap for the "warning is disabled" case .
>
> I'll try bootstrapping the attached patch.
Will you commit the attached patch? (A little confused…)
Qing
>
> Hope this makes sense.
> Dave
> <0001-diagnostics-add-class-deferred_diagnostic_path.patch>
