Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill <jason@redhat.com>]

On 06/02/2017 05:28 PM, Martin Sebor wrote:
> On 05/31/2017 05:34 PM, Jason Merrill wrote:
>> On 05/27/2017 06:44 PM, Martin Sebor wrote:
>>> +  /* True if the class is trivial and has a trivial non-deleted copy
>>> +     assignment, copy ctor, and default ctor, respectively.  The last
>>> +     one isn't used to issue warnings but only to decide what suitable
>>> +     alternatives to offer as replacements for the raw memory
>>> operation.  */
>>> +  bool trivial = trivial_type_p (desttype);
>>
>> This comment seems out of date; there's only one variable here now.
>>
>>> +  /* True if the class is has a non-deleted trivial assignment.  Set
>>
>> s/is//
>>
>>> +  /* True if the class has a (possibly deleted) trivial copy ctor.  */
>>> +  bool trivcopy = trivially_copyable_p (desttype);
>>
>> "True if the class is trivially copyable."
>>
>>> +      if (delassign)
>>> +        warnfmt = G_("%qD writing to an object of type %#qT with "
>>> +             "deleted copy assignment");
>>> +      else if (!trivassign)
>>> +        warnfmt = G_("%qD writing to an object of type %#qT with "
>>> +             "no trivial copy assignment");
>>> +      else if (!trivial)
>>> +        warnfmt = G_("%qD writing to an object of non-trivial "
>>> +             "type %#qT; use assignment instead");
>>
>> I'd still like the !trivial test to come first in all the memset cases,
>> !trivcopy in the copy cases.
> 
> The tests are in the order they're in to provide as much useful
> detail in the diagnostics as necessary to understand the problem
> make the suggestion meaningful.  To what end you want to change
> it?

Mostly I was thinking that whether a class is trivial(ly copyable) is 
more to the point, but I guess what you have now is fine.

>>> +static bool
>>> +has_trivial_special_function (tree ctype, special_function_kind sfk,
>>> +                  bool *deleted_p)
>>
>> This seems redundant with type_has_trivial_fn.  If that function is
>> giving the wrong answer for a class where all of the SFK are deleted,
>> let's fix that, under check_bases_and_members, rather than introduce a
>> new function.  I don't want to call synthesized_method_walk every time
>> we want to check whether a function is trivial.
> 
> A deleted special function can be trivial.

I believe that in the language, the triviality of a deleted function 
cannot be determined.  But I believe you're right about the behavior of 
type_has_trivial_fn, which is why I mentioned changing it.

> Maybe I should use a different approach and instead of trying
> to see if a function is deleted use trivially_xible to see if
> it's usable.  That will mean changing the diagnostics from
> "with a deleted special function" to "without trivial special
> function" but it will avoid calling synthesized_method_walk
> while still avoiding giving bogus suggestions.
> 
> Is this approach acceptable?

Yes, that makes sense.

Jason