Closed shemeshg closed 1 year ago
Hi @shemeshg, do you have a minimal code example for the case you are describing? That would be very helpful. Thanks.
//NOLING
to suppress the errorYou can assume QMenu *fileMenu = new QMenu("File");
is actually gsl::owner<QMenu>("File");
I did not used gsl::owner
here because then it will complain I did not freed that memory....
https://github.com/shemeshg/RtMidiWrap/blob/34992346a2fd32b612f231582fe99c77d7bbf08a/src/uimain.cpp
QMenuBar* menuBar = new QMenuBar(); //NOLINT
QMenu *fileMenu = new QMenu("File");
QAction *aboutAct = new QAction(tr("About"), this);
aboutAct->setStatusTip(tr("Show the application's About box"));
connect(aboutAct, &QAction::triggered, this, &UiMain::about);
QAction *aboutQtAct = new QAction(tr("About Qt"), this);
aboutQtAct->setStatusTip(tr("Show the Qt library's About box"));
connect(aboutQtAct, &QAction::triggered, this, &QApplication::aboutQt);
QAction *quitQtAct = new QAction(tr("Quit"), this);
quitQtAct->setStatusTip(tr("Quit"));
connect(quitQtAct, &QAction::triggered, this, &QApplication::quit);
fileMenu->addAction(aboutAct);
fileMenu->addAction(aboutQtAct);
fileMenu->addAction(quitQtAct);
menuBar->addMenu(fileMenu);
mainLayout->setMenuBar(menuBar);
menuBar
is not required to be deleted of freed explicitly, since submitting to parent mainLayout->setMenuBar(menuBar);
already does it all., using unique pointer
or explicit destructor, would cause it to be deleted twice...Here example of the QT documentation web site https://doc.qt.io/qt-6/qtwidgets-mainwindows-menus-example.html#:~:text=the%20QActionGroup%20class.-,MainWindow,-Class%20Implementation
MainWindow::MainWindow()
{
QWidget *widget = new QWidget;
setCentralWidget(widget);
QWidget *topFiller = new QWidget;
topFiller->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);
infoLabel = new QLabel(tr("<i>Choose a menu option, or right-click to "
"invoke a context menu</i>"));
infoLabel->setFrameStyle(QFrame::StyledPanel | QFrame::Sunken);
infoLabel->setAlignment(Qt::AlignCenter);
QWidget *bottomFiller = new QWidget;
bottomFiller->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);
QVBoxLayout *layout = new QVBoxLayout;
layout->setContentsMargins(5, 5, 5, 5);
layout->addWidget(topFiller);
layout->addWidget(infoLabel);
layout->addWidget(bottomFiller);
widget->setLayout(layout);
WxWidget example (from ducumentation) https://docs.wxwidgets.org/latest/overview_helloworld.html#:~:text=with%20respective%20calls.-,MyFrame,-%3A%3AMyFrame()
MyFrame::MyFrame()
: wxFrame(NULL, wxID_ANY, "Hello World")
{
wxMenu *menuFile = new wxMenu;
menuFile->Append(ID_Hello, "&Hello...\tCtrl-H",
"Help string shown in status bar for this menu item");
menuFile->AppendSeparator();
menuFile->Append(wxID_EXIT);
wxMenu *menuHelp = new wxMenu;
menuHelp->Append(wxID_ABOUT);
wxMenuBar *menuBar = new wxMenuBar;
menuBar->Append(menuFile, "&File");
menuBar->Append(menuHelp, "&Help");
SetMenuBar(menuBar);
CreateStatusBar();
SetStatusText("Welcome to wxWidgets!");
... continued below ...
https://code.qt.io/cgit/qt/qtbase.git/tree/examples/widgets/mainwindows/menus?h=6.4
Thanks Shemeshg
Hi @shemeshg,
Please confirm if my understanding is correct. You have a couple of distinct concerns here:
1)
You are using an API outside of your control which intends to consume ownership of a pointer, but does not explicitly use the gsl::owner
annotation (for instance, QMainWindow::setMenuBar(QMenuBar *menuBar)
). You would like to keep using gsl::owner
in your own code, and communicate to the ownership checker that this function does take ownership.
2) You would like for way to transfer function-local ownership into a non-function-local scope, while not invalidating the original owner pointer.
struct A
{
gsl::owner<int*> class_level_owner;
void takeOwnership(gsl::owner<int*> owner) {
class_level_owner = owner;
}
};
void foo(A a)
{
gsl::owner<int*> my_owner = new int(42);
a.takeOwnership(my_owner);
++(*my_owner); // valid, if we say that my_owner is not "moved"
}
3) You would like the behavior in (2) to also work for the cases in (1)
This exactly describe the problem. Especially as you said
++(*my_owner); // valid, if we say that my_owner is not "moved"
is the essence of it, since in QT programmers keeps referencing and changing my_owner
while class_level_owner
kept private and used for `parental memory management' only..
Hi @shemeshg,
With the current model of GSL owner, passing an owner to a function will unavoidably transfer ownership into that function. This applies to both free functions and class methods.
void bar(gsl::owner<int*> o);
struct A
{
void bar(gsl::owner<int*> o);
};
void foo1()
{
gsl::owner<int*> ptr = new int(42);
bar(ptr); // ptr has now transferred ownership into bar's parameter o
++(*ptr); // BAD
}
void foo2()
{
gsl::owner<int*> ptr = new int(42);
A a;
a.bar(ptr); // ptr has now transferred ownership into bar's parameter o
++(*ptr); // BAD
}
This is the expected behavior, because the ownership model intends for the checks to perform non-global static analysis. The checker will not look into the body of bar
in the general case, and so will not attempt to differentiate between implementations of bar
such as:
A::bar(gsl::owner<int*> o) { my_class_member_ = o; }
// versus
A::bar(gsl::owner<int*> o) { delete o; }
// - for this implementation, calling `bar(my_owner);` and then `++(*my_owner);` is bad!
So the answer to your question (2) is: no, this is not possible. If you want to keep using the pointer after it has been passed to the function, you will have to re-acquire it somehow from QMenuBar.
Without proper gsl::owner
annotations, it is impossible to avoid generating ownership warnings. However, if you want to preserve as many ownership semantics as possible, and minimize the locations where you need to add suppressions, you could create a simple wrapper:
void addMenu(QMenuBar *menuBar, gsl::owner<QMenu*> fileMenu)
{
// add NOLINT where appropriate
menuBar->addMenu(fileMenu);
}
This will allow ownership errors to still be caught at the callsites:
// example ownership mistake
void maybeAddFileMenu(QMenuBar* menuBar, bool condition)
{
gsl::owner<QMenu*> fileMenu = new QMenu("File");
if (condition) {
addMenu(menuBar, fileMenu); // ownership transferred in this branch, OK
}
// BAD, forgot to delete fileMenu if condition == false
// owner checker will raise warning
}
If these solutions are not desirable, then the ownership model is most likely not compatible with this existing Qt API.
Hi
Thanks for the detailed response, The abstraction suggested will not work, since properties in most cases stored on a class.
This would cause the Linter to expects delete in the destructor (and no delete
used in Qt).
uimain.h:17:5: member variable of type 'gsl::owner<>' requires the class 'UiMain' to implement a destructor to release the owned resource [cppcoreguidelines-owning-memory]
Thanks anyway, I appreciate very much the effort.
For now I'll just keep using //NOLINT at the new
statement, hopefully maybe in the future there will be Owner annotation for that.
Are you still referring to QMenuBar* menuBar = new QMenuBar(); //NOLINT
? Looking at the code in uimain.cpp
I think QGridLayout *mainLayout = new QGridLayout;
and QMenuBar* menuBar = new QMenuBar();
can use the owner
annotation using my suggestions above, since they are not class members. But that is up to you.
In Qt, memory managed by parent and transferred by pointer. How do I indicate that the GSL::owner() is now managed by the parent (but not moved). [Maybe GSL:owner() may turn in to GSL::ownerParent<> for example]
This means that the
This is important since