Date: Mon, 16 Jan 2023 19:23:44 +0100
pon., 16 sty 2023 o 16:32 Julien Villemure-Fréchette via
Std-Discussion <std-discussion_at_[hidden]> napisał(a):
>
> The most plausible solution that comes to my mind, is to use std::unique_pointer to create objects and then,
> release ownership exactly at the call sites of 'ownership transferring' member functions. Handling dynamically allocated memory in a *local variable* of raw pointer type *is* a memory leak, if any evaluation could throw before this variable is handed to a proper owner.
>
> If we transform the previous examples:
>
> 1)
>
> auto parent = std::make_unique<QWidget>();
>
> // If the construction fails, then the `parent` variable will not leak (this was not the case if it was a raw pointer)
> auto w = std::make_unique<QWidget>();
> // release the ownership of the parent variable
> w->setParent(parent.release()); // ownership transfer
>
> 2)
>
> auto parent = std::make_unique<QWidget>();
> // no more potential leaks
> auto w = std::make_unique<QWidget>();
> // no more potential leaks
> std::unique_ptr<QLayout> l{ new QVBoxLayout};
>
>
> parent->setLayout(l.release()); // "ownership transfer"
>
>
> l->addWidget(w.release()); //
>
> 3)
>
> auto parent = std::make_unique<QWidget>();
> auto w = std::make_unique<QWidget>();
>
> std::unique_ptr<QLayout> l{ new QVBoxLayout};
>
> l->addWidget(w.get()); // `w` still without an owner.
>
> parent->setLayout(l.release()); // `l` and `w`'s ownerships get both transferred
>
This can't work, how `w` could know if it should release ownership?
Overall I think this API is broken when consider ownership:
```
void foo(QLayout& l)
{
l.addWidget(new QWidget());
}
```
Is this code memory leak or not? This is impossible to say, because it
all depends on
how `l` is used. You can't locally reason about it.
Another is what if `setLayout` or `addWidget` throws before any
transfer takes place?
Only way I could see this could work is to add
`QVBoxLayoutAsTemporalParent` that deletes
all linked widgets when it was not linked to any parent widget.
With this this class could benefit from all `unique_ptr` conventions
isted working against it.
```
void foo(QVBoxLayoutAsTemporalParent& l)
{
l.addWidget(std::make_unique<QWidget>());
}
```
Now ownership is clear and deterministic, if `l` is not linked to
anything it will delete all linked widgets, but if is linked to parent
then it will delete widgets.
If any operation fails no memory will be lost.
> Julien V.
>
> On 2023-01-16 7:23 a.m., Johannes Schaub via Std-Discussion wrote:
>
> Thanks for the detailed answer, I see where the problems are now.
>
> What would you recommend for Qt codebases that use the guideline checks of static analyzers to tell them about these special relations short of decorating all the sites with a NOLINT comment? For example, clang gives lots of warnings about those cases. If pointers to QObject derived objects are involved, disable all checks? Hope there's an easy way to tweak those checks.
>
> Am Mo., 16. Jan. 2023 um 13:10 Uhr schrieb Giuseppe D'Angelo via Std-Discussion <std-discussion_at_[hidden]>:
>>
>> On 16/01/2023 12:21, Johannes Schaub via Std-Discussion wrote:
>> > For example, can it be used in Qt for the following?
>> >
>> > |gsl::owner<QWidget*> w{new QWidget{parent}} |
>> >
>> > In this example, the ownership is shared by the new-site and the
>> > |parent|, because the code who has |new|-ed the object can |delete| |w|,
>> > and |W|'s destructor will take itself out of |parent| children list.
>> > However, if |w| is not deleted at the new-site, then |parent| will
>> > delete it in its destructor. It is not enforced by the type system that
>> > there are no new-site users left, so I'm not sure about the ownership
>> > kind here.
>> >
>> > 1. Is this an example of shared ownership, and
>> > 2. can |gsl::owner| be used for it?
>> >
>>
>> We had quite some debates in Qt about how to properly mark these
>> pointers and I don't think we ended up with a decent outcome.
>>
>>
>> But gsl::owner is the wrong choice here. From the guidelines:
>>
>> > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-views
>>
>> > owner<T*> // a T* that owns the object pointed/referred
>>
>> > owner is used to mark owning pointers in code
>>
>> > An owner<T> differs from a resource handle for a T by still requiring an explicit delete.
>>
>> The problem is that you're not owning `w`. If a static analysis tool
>> doesn't see a further ownership transfer of `w` (but you just drop it on
>> the floor) nor a delete, then the tool is allowed to emit a diagnostic
>> -- you're leaking it! But that's false.
>>
>> In my experience with big Qt codebases, I'd even claim that you could
>> use a raw pointer there, with the intent of express _lack_ of ownership.
>> While `delete w` is perfectly legal and does the right thing (creating
>> the interesting ownership model that you mention), in practice no one
>> does it.
>>
>> I'm not sure if I'd call it "shared ownership", as normally one uses
>> that term to refer to scenarios (shared_ptr) where as long as there is
>> at least one owner, a resource can't be destroyed. This looks more like
>> a "weak shared ownership" model, where one owner can destroy a resource
>> at any time, but the other owners will know about it and don't use it
>> any more / don't double delete.
>>
>>
>> To compound the effect, the Qt APIs give developers tremendous amounts
>> of leeway but ultimately end up "doing the right thing". I don't think
>> there can be a universal approach here without also bringing in some
>> strict coding guidelines for your own project. Examples:
>>
>>
>> 1)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>> w->setParent(parent); // "ownership transfer" of `w`, after
>> construction. What do you decorate with gsl::owner here?
>>
>>
>> 2)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>>
>> QLayout *l = new QVBoxLayout;
>> parent->setLayout(l); // "ownership transfer"
>>
>> l->addWidget(w); // `w`'s ownership is transferred to `parent`,
>> // not to `l`!
>>
>> delete l; // OK. `w` still owned by `parent`
>>
>> 3)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>>
>> QLayout *l = new QVBoxLayout;
>>
>> l->addWidget(w); // `w` still without an owner
>>
>> parent->setLayout(l); // `l` and `w`'s ownerships get both transferred
>>
>>
>>
>> My 2 c,
>> --
>> Giuseppe D'Angelo | giuseppe.dangelo_at_[hidden] | Senior Software Engineer
>> KDAB (France) S.A.S., a KDAB Group company
>> Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com
>> KDAB - The Qt, C++ and OpenGL Experts
>>
>> --
>> Std-Discussion mailing list
>> Std-Discussion_at_[hidden]
>> https://lists.isocpp.org/mailman/listinfo.cgi/std-discussion
>
>
> --
> Std-Discussion mailing list
> Std-Discussion_at_[hidden]
> https://lists.isocpp.org/mailman/listinfo.cgi/std-discussion
Std-Discussion <std-discussion_at_[hidden]> napisał(a):
>
> The most plausible solution that comes to my mind, is to use std::unique_pointer to create objects and then,
> release ownership exactly at the call sites of 'ownership transferring' member functions. Handling dynamically allocated memory in a *local variable* of raw pointer type *is* a memory leak, if any evaluation could throw before this variable is handed to a proper owner.
>
> If we transform the previous examples:
>
> 1)
>
> auto parent = std::make_unique<QWidget>();
>
> // If the construction fails, then the `parent` variable will not leak (this was not the case if it was a raw pointer)
> auto w = std::make_unique<QWidget>();
> // release the ownership of the parent variable
> w->setParent(parent.release()); // ownership transfer
>
> 2)
>
> auto parent = std::make_unique<QWidget>();
> // no more potential leaks
> auto w = std::make_unique<QWidget>();
> // no more potential leaks
> std::unique_ptr<QLayout> l{ new QVBoxLayout};
>
>
> parent->setLayout(l.release()); // "ownership transfer"
>
>
> l->addWidget(w.release()); //
>
> 3)
>
> auto parent = std::make_unique<QWidget>();
> auto w = std::make_unique<QWidget>();
>
> std::unique_ptr<QLayout> l{ new QVBoxLayout};
>
> l->addWidget(w.get()); // `w` still without an owner.
>
> parent->setLayout(l.release()); // `l` and `w`'s ownerships get both transferred
>
This can't work, how `w` could know if it should release ownership?
Overall I think this API is broken when consider ownership:
```
void foo(QLayout& l)
{
l.addWidget(new QWidget());
}
```
Is this code memory leak or not? This is impossible to say, because it
all depends on
how `l` is used. You can't locally reason about it.
Another is what if `setLayout` or `addWidget` throws before any
transfer takes place?
Only way I could see this could work is to add
`QVBoxLayoutAsTemporalParent` that deletes
all linked widgets when it was not linked to any parent widget.
With this this class could benefit from all `unique_ptr` conventions
isted working against it.
```
void foo(QVBoxLayoutAsTemporalParent& l)
{
l.addWidget(std::make_unique<QWidget>());
}
```
Now ownership is clear and deterministic, if `l` is not linked to
anything it will delete all linked widgets, but if is linked to parent
then it will delete widgets.
If any operation fails no memory will be lost.
> Julien V.
>
> On 2023-01-16 7:23 a.m., Johannes Schaub via Std-Discussion wrote:
>
> Thanks for the detailed answer, I see where the problems are now.
>
> What would you recommend for Qt codebases that use the guideline checks of static analyzers to tell them about these special relations short of decorating all the sites with a NOLINT comment? For example, clang gives lots of warnings about those cases. If pointers to QObject derived objects are involved, disable all checks? Hope there's an easy way to tweak those checks.
>
> Am Mo., 16. Jan. 2023 um 13:10 Uhr schrieb Giuseppe D'Angelo via Std-Discussion <std-discussion_at_[hidden]>:
>>
>> On 16/01/2023 12:21, Johannes Schaub via Std-Discussion wrote:
>> > For example, can it be used in Qt for the following?
>> >
>> > |gsl::owner<QWidget*> w{new QWidget{parent}} |
>> >
>> > In this example, the ownership is shared by the new-site and the
>> > |parent|, because the code who has |new|-ed the object can |delete| |w|,
>> > and |W|'s destructor will take itself out of |parent| children list.
>> > However, if |w| is not deleted at the new-site, then |parent| will
>> > delete it in its destructor. It is not enforced by the type system that
>> > there are no new-site users left, so I'm not sure about the ownership
>> > kind here.
>> >
>> > 1. Is this an example of shared ownership, and
>> > 2. can |gsl::owner| be used for it?
>> >
>>
>> We had quite some debates in Qt about how to properly mark these
>> pointers and I don't think we ended up with a decent outcome.
>>
>>
>> But gsl::owner is the wrong choice here. From the guidelines:
>>
>> > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-views
>>
>> > owner<T*> // a T* that owns the object pointed/referred
>>
>> > owner is used to mark owning pointers in code
>>
>> > An owner<T> differs from a resource handle for a T by still requiring an explicit delete.
>>
>> The problem is that you're not owning `w`. If a static analysis tool
>> doesn't see a further ownership transfer of `w` (but you just drop it on
>> the floor) nor a delete, then the tool is allowed to emit a diagnostic
>> -- you're leaking it! But that's false.
>>
>> In my experience with big Qt codebases, I'd even claim that you could
>> use a raw pointer there, with the intent of express _lack_ of ownership.
>> While `delete w` is perfectly legal and does the right thing (creating
>> the interesting ownership model that you mention), in practice no one
>> does it.
>>
>> I'm not sure if I'd call it "shared ownership", as normally one uses
>> that term to refer to scenarios (shared_ptr) where as long as there is
>> at least one owner, a resource can't be destroyed. This looks more like
>> a "weak shared ownership" model, where one owner can destroy a resource
>> at any time, but the other owners will know about it and don't use it
>> any more / don't double delete.
>>
>>
>> To compound the effect, the Qt APIs give developers tremendous amounts
>> of leeway but ultimately end up "doing the right thing". I don't think
>> there can be a universal approach here without also bringing in some
>> strict coding guidelines for your own project. Examples:
>>
>>
>> 1)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>> w->setParent(parent); // "ownership transfer" of `w`, after
>> construction. What do you decorate with gsl::owner here?
>>
>>
>> 2)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>>
>> QLayout *l = new QVBoxLayout;
>> parent->setLayout(l); // "ownership transfer"
>>
>> l->addWidget(w); // `w`'s ownership is transferred to `parent`,
>> // not to `l`!
>>
>> delete l; // OK. `w` still owned by `parent`
>>
>> 3)
>>
>> QWidget *parent = new QWidget;
>> QWidget *w = new QWidget;
>>
>> QLayout *l = new QVBoxLayout;
>>
>> l->addWidget(w); // `w` still without an owner
>>
>> parent->setLayout(l); // `l` and `w`'s ownerships get both transferred
>>
>>
>>
>> My 2 c,
>> --
>> Giuseppe D'Angelo | giuseppe.dangelo_at_[hidden] | Senior Software Engineer
>> KDAB (France) S.A.S., a KDAB Group company
>> Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com
>> KDAB - The Qt, C++ and OpenGL Experts
>>
>> --
>> Std-Discussion mailing list
>> Std-Discussion_at_[hidden]
>> https://lists.isocpp.org/mailman/listinfo.cgi/std-discussion
>
>
> --
> Std-Discussion mailing list
> Std-Discussion_at_[hidden]
> https://lists.isocpp.org/mailman/listinfo.cgi/std-discussion
Received on 2023-01-16 18:23:57