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

[Martin Sebor <msebor at gmail dot com>]

On 05/19/2017 03:42 PM, Jason Merrill wrote:
> On Fri, May 19, 2017 at 4:07 PM, Martin Sebor <msebor@gmail.com> wrote:
> > On 05/19/2017 01:07 PM, Jason Merrill wrote:
> > > On Tue, May 16, 2017 at 5:39 PM, Martin Sebor <msebor@gmail.com> wrote:
> > > > On 05/16/2017 01:41 PM, Jason Merrill wrote:
> > > >
> > > > > I'm still not convinced we need to consider standard-layout at all.
> > > >
> > > >
> > > > I agree.  The patch doesn't make use of is_standard_layout_p().
> > > > It defines its own helper called almost_std_layout_p() that
> > > > combines trivial_type_p() with tests for non-public members,
> > >
> > >
> > > That's the part that seems unnecessary.  Why do we care about
> > > non-public members?
> >
> > Because modifying them breaks encapsulation.
>
> Not if you're clearing/copying the object as a whole.
>
> > If I take a legacy struct, make some of its members private,
> > and define accessors and modifiers to manipulate those members
> > and maintain invariants between them, I will want to check and
> > adjust all code that changes objects of the struct in ways that
> > might violate the invariants.
>
> For a trivial type, worrying about invariants doesn't make sense to
> me, since default-initialization won't establish any invariants.  And
> bzero or memset(0) will have the same effect as value-initialization
> (if zero_init_p (type); we probably want to check that).  If you're
> going to establish invariants, I would expect you to write a default
> constructor, which would make the class non-trivial.

Thanks for the zero_init_p pointer!  Let me add that to the patch
along with Pedro's suggestion to use the current C++ terminology,
retest and resubmit.

In most cases you're right that defining the default constructor
is the way to go.  There is are a couple of use cases where a ctor
tends to be avoided: when objects the class need to be initialized
statically, and where they need to be PODs.  GCC itself relies on
the latter (e.g., some of the vec templates), apparently because
it stores them in unions.  It doesn't look tome like these vec
templates maintain much of an invariant of any kind, but they
easily could.

An example of the static initialization case is an atomic class
(not necessarily std::atomic though I think this would apply to
it as well).  Its objects usually need to be statically default-
initializable (without running any ctors) but then they must be
explicitly initialized by some sort of an init call to be made
valid, and their state can only be accessed and modified via
member functions (and so their state is private).  Modifying
them by any other means, including by memset or memcpy, is
undefined.

> > What common use case are you concerned about that isn't more
> > appropriately expressed using the generated default or copy
> > constructor or assignment operator?
>
> None; I am concerned about focusing the warning on code that is
> actually likely to be problematic.

Hopefully the above explanation resolves that concern.  If not,
please let me know.

Martin