On Mon, 26 Sept 2022 at 15:20, Sébastien Bini <sebastien.bini_at_[hidden]>
wrote:

> On Mon, Sep 26, 2022 at 2:03 PM Edward Catmur <ecatmur_at_[hidden]>
> wrote:
>
>> On Mon, 26 Sept 2022 at 12:28, Sébastien Bini <sebastien.bini_at_[hidden]>
>> wrote:
>>
>>> On Mon, Sep 26, 2022 at 12:31 PM Edward Catmur <ecatmur_at_[hidden]>
>>> wrote:
>>>
>>>> On Mon, 26 Sept 2022 at 10:36, Sébastien Bini <sebastien.bini_at_[hidden]>
>>>> wrote:
>>>>
>>>>> My bad, I meant no user-provided *relocation, move or copy*
>>>>> constructors.
>>>>>
>>>>
>>>> Sure. I'd be a bit concerned that this is too restrictive, though it
>>>> shouldn't be a problem for tuple types as far as I can tell, but they might
>>>> want to user-provide special member functions for logging? This should be
>>>> called out, though.
>>>>
>>>
>>> I wouldn't know otherwise how to have the guarantee that all subobjects
>>> are independent from one-another. I also wonder whether we should impose no
>>> user-provided default constructor.
>>>
>>
>> I think it's one or the other; if access is checked then the code
>> accessing the private/protected base/member is taking responsibility for
>> maintaining any relevant invariants. So long as access checking is
>> bulletproof I don't think we need to worry too much about a class breaking
>> its own invariants; the union hack would always be a possibility. I'd
>> remove that requirement (no special member functions) and say that if you
>> break invariants by decomposing into private bases and members that's your
>> own responsibility.
>>
>
> I agree with the class invariant part.
> But the no user-provided reloc, move and copy ctor clause was not
> motivated because of the invariant, but because we wanted to be sure that
> subobjects of a class could be independently relocated or destroyed. So at
> minimum we must have a default destructor. And that's not enough.
>
> struct PainterGuard
> {
> Painter* _p;
> StateGuard(Painter& p) : _p{&p} { _p->save(); }
> ~StateGuard() { _p->restore(); }
> };
>
> class PainterWithGuard
> {
> public:
> PainterWithGuard(Painter p) : _p{reloc p}, _guard{_p} {}
> PainterWithGuard() : _guard{_p} {}
> PainterWithGuard(PainterWithGuard) /* reloc ctor */ { _guard._p = &_p;
> }
> public:
> Painter _p;
> private:
> PainterGuard _guard;
> };
>
> This is a perfectly valid class. Its default destructor works fine.
> Unfortunately, from outside the class (with no access to private
> data-members), doing:
> `auto [p] = std::decompose<&PainterWithGuard::_p>(reloc painterWithGuard);`
> Will call `restore()` on a destructed/relocated object.
>
> Likewise doing (in the class implementation this time):
> `auto [p, g] = std::decompose<&PainterWithGuard::_p,
> &PainterWithGuard::_guard>(reloc painterWithGuard);`
> Will construct a PainterGuard that points to invalid data. This one can be
> incriminated to the class writer as it needs access to private data.
>
> I agree this class has a bad design, but its safe to use otherwise.
> std::decompose shouldn't cause crashes on badly designed classes.
>
> IMO the no user-provided reloc, move and copy ctor clause gives us that
> extra guarantee, that there are no relationships between subobjects and as
> such it is safe to independently relocate or destroy them.
>

Great example, and that's the sort of thing I was trying to describe with
my talk of private inheritance. (PainterGuard could be a private base of
PainterWithGuard, perhaps.)

But I can still write a similar class that is currently safe, has no
user-defined special member functions but would be unsafe to decompose:
instead of writing or deleting the relocating constructor, have
PainterGuard inherit from boost::noncopyable. So the existence of
user-defined special member functions is insufficient to determine whether
a class is safe to decompose.

Instead, consider: `std::decompose` is accessing (on behalf of its caller)
each direct subobject (base and data member) that is returned. But it is
also accessing the *other* direct subobjects that it does not return, in
order to destroy them. So let's say that to call std::decompose, you must
have access to each direct subobject, including those that you don't
request. (Plus their relocators or destructors, respectively.)

Then `auto [p] = std::decompose<&PainterWithGuard::_p>(reloc
painterWithGuard);` would be ill-formed because in that context,
`painterWithGuard._guard` is ill-formed.

Do you think this would work?