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

[Martin Sebor <msebor at gmail dot com>]

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?

AFAICS, all it will accomplish is shuffle the tests around
because starting with !trivial means I'll still need to test
for delassign and delcopy before issuing the warning so that
I can include the right suggestion (and avoid suggesting to
use assignment when it's not available).

> > +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.  type_has_trivial_fn()
only determines whether a function is trivial, not whether it's deleted. 
 I chose not to use the function because when a special
function is deleted I don't want to have the warning suggest it
as an alternative to the raw memory function.

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?

Martin