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

[Jason Merrill <jason at redhat dot 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