Long live [[nodiscard]] QFile::open
Having already caught some bugs in real code because of unchecked calls to QFile::open, this commit marks QFile::open (and open() in other file-I/O classes) as [[nodiscard]]. Since it's going to raise warnings, the plan is to keep the existing behavior up to and including the next LTS. Then the warnings will switch on by default. All of this is protected by system of macros to opt-in or opt-out the behavioral change at any time. A possible counter-argument for doing this is that QFile::open is also used for opening files in the the resource system, and that opening "cannot fail". It clearly can, if the resource is moved away or renamed; code should at a minimum use a Q_ASSERT in debug builds. Another counter-argument is the opening of file handles or descriptors; but again, that opening may fail in case the handle has been closed or if the flags are incompatible. --- Why not marking *every* open() override? Because some are not meant to be called directly -- for instance sockets are supposed to be open via calls to `connectToHost` or similar. One notable exception is QIODevice::open() itself. Although rarely called directly by user code (which just calls open() on a specific subclass, which likely has an override), it may be called: 1) By code that just takes a `QIODevice *` and does something with it. That code is arguably more rare than code using QFile directly. Still, being "generic" code, they have an extra responsibility when making sure to handle a possible opening failure. 2) By QIODevice subclasses, which are even more rare. However, they usually ignore the return from QIODevice::open() as it's unconditionally true. (QIODevice::open() doesn't use the protected virtual pattern.) I'll try and tackle QIODevice in a future commit. [ChangeLog][QtCore][QFileDevice] The open() functions of file-related I/O classes (such as QFile, QSaveFile, QTemporaryFile) can now be marked with the "nodiscard" attribute, in order to prevent a category of bugs where the return value of open() is not checked and the file is then used. In order to avoid warnings in existing code, the marking can be opted in or out, by defining QT_USE_NODISCARD_FILE_OPEN or the QT_NO_USE_NODISCARD_FILE_OPEN macros. By default, Qt will automatically enable nodiscard on these functions starting from Qt 6.10. Change-Id: Ied940e1c0a37344f5200b2c51b05cd1afcb2557d Reviewed-by: Thiago Macieira <thiago.macieira@intel.com> Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
This commit is contained in:
parent
067e3ec1bf
commit
7466831509
@ -28,6 +28,7 @@ qt_internal_add_module(Concurrent
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::CorePrivate
|
||||
PUBLIC_LIBRARIES
|
||||
|
@ -323,6 +323,7 @@ qt_internal_add_module(Core
|
||||
QT_NO_QPAIR
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_TYPESAFE_FLAGS
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
"${CMAKE_CURRENT_BINARY_DIR}/global"
|
||||
"${CMAKE_CURRENT_BINARY_DIR}/kernel" # for moc_qobject.cpp to be found by qobject.cpp
|
||||
|
@ -346,6 +346,7 @@ bool QInternal::activateCallbacks(Callback cb, void **parameters)
|
||||
\row \li 6.6.0 \li The qExchange() function (see \l{QT_NO_QEXCHANGE})
|
||||
\row \li 6.7.0 \li Overloads of QObject::connect that do not take a context object (see \l{QT_NO_CONTEXTLESS_CONNECT})
|
||||
\row \li 6.8.0 \li The qAsConst() function (see \l{QT_NO_QASCONST})
|
||||
\row \li 6.8.0 \li File-related I/O classes have their \c{open()} functions marked \c{[[nodiscard]]} (see \l{QT_USE_NODISCARD_FILE_OPEN})
|
||||
\endtable
|
||||
|
||||
Moreover, individual APIs may also get disabled as part of the
|
||||
|
@ -199,6 +199,9 @@ namespace QT_NAMESPACE {}
|
||||
|
||||
#if QT_ENABLE_STRICT_MODE_UP_TO >= QT_VERSION_CHECK(6, 8, 0)
|
||||
# define QT_NO_QASCONST
|
||||
# if !defined(QT_USE_NODISCARD_FILE_OPEN) && !defined(QT_NO_USE_NODISCARD_FILE_OPEN)
|
||||
# define QT_USE_NODISCARD_FILE_OPEN
|
||||
# endif
|
||||
#endif // 6.8.0
|
||||
#endif // QT_ENABLE_STRICT_MODE_UP_TO
|
||||
|
||||
|
@ -897,6 +897,8 @@ QFile::copy(const QString &fileName, const QString &newName)
|
||||
of the file name, otherwise, it won't be possible to create this
|
||||
non-existing file.
|
||||
|
||||
\sa QT_USE_NODISCARD_FILE_OPEN
|
||||
|
||||
\sa QIODevice::OpenMode, setFileName()
|
||||
*/
|
||||
bool QFile::open(OpenMode mode)
|
||||
@ -941,7 +943,7 @@ bool QFile::open(OpenMode mode)
|
||||
such permissions will generate warnings when the Security tab of the Properties dialog
|
||||
is opened. Granting the group all permissions granted to others avoids such warnings.
|
||||
|
||||
\sa QIODevice::OpenMode, setFileName()
|
||||
\sa QIODevice::OpenMode, setFileName(), QT_USE_NODISCARD_FILE_OPEN
|
||||
\since 6.3
|
||||
*/
|
||||
bool QFile::open(OpenMode mode, QFile::Permissions permissions)
|
||||
@ -998,7 +1000,7 @@ bool QFile::open(OpenMode mode, QFile::Permissions permissions)
|
||||
you cannot use this QFile with a QFileInfo.
|
||||
\endlist
|
||||
|
||||
\sa close()
|
||||
\sa close(), QT_USE_NODISCARD_FILE_OPEN
|
||||
|
||||
\b{Note for the Windows Platform}
|
||||
|
||||
@ -1064,7 +1066,7 @@ bool QFile::open(FILE *fh, OpenMode mode, FileHandleFlags handleFlags)
|
||||
\warning Since this function opens the file without specifying the file name,
|
||||
you cannot use this QFile with a QFileInfo.
|
||||
|
||||
\sa close()
|
||||
\sa close(), QT_USE_NODISCARD_FILE_OPEN
|
||||
*/
|
||||
bool QFile::open(int fd, OpenMode mode, FileHandleFlags handleFlags)
|
||||
{
|
||||
|
@ -282,10 +282,10 @@ public:
|
||||
}
|
||||
#endif // QT_CONFIG(cxx17_filesystem)
|
||||
|
||||
bool open(OpenMode flags) override;
|
||||
bool open(OpenMode flags, Permissions permissions);
|
||||
bool open(FILE *f, OpenMode ioFlags, FileHandleFlags handleFlags=DontCloseHandle);
|
||||
bool open(int fd, OpenMode ioFlags, FileHandleFlags handleFlags=DontCloseHandle);
|
||||
QFILE_MAYBE_NODISCARD bool open(OpenMode flags) override;
|
||||
QFILE_MAYBE_NODISCARD bool open(OpenMode flags, Permissions permissions);
|
||||
QFILE_MAYBE_NODISCARD bool open(FILE *f, OpenMode ioFlags, FileHandleFlags handleFlags=DontCloseHandle);
|
||||
QFILE_MAYBE_NODISCARD bool open(int fd, OpenMode ioFlags, FileHandleFlags handleFlags=DontCloseHandle);
|
||||
|
||||
qint64 size() const override;
|
||||
|
||||
|
@ -168,6 +168,35 @@ void QFileDevicePrivate::setError(QFileDevice::FileError err, int errNum)
|
||||
handle is left open when the QFile object is destroyed.
|
||||
*/
|
||||
|
||||
/*!
|
||||
\macro QT_USE_NODISCARD_FILE_OPEN
|
||||
\macro QT_NO_USE_NODISCARD_FILE_OPEN
|
||||
\relates QFileDevice
|
||||
\since 6.8
|
||||
|
||||
File-related I/O classes (such as QFile, QSaveFile, QTemporaryFile)
|
||||
have an \c{open()} method to open the file they act upon. It is
|
||||
important to check the return value of the call to \c{open()}
|
||||
before proceeding with reading or writing data into the file.
|
||||
|
||||
For this reason, starting with Qt 6.8, some overloads of \c{open()}
|
||||
have been marked with the \c{[[nodiscard]]} attribute. Since this
|
||||
change may raise warnings in existing codebases, user code can
|
||||
opt-in or opt-out from having the attribute applied by defining
|
||||
certain macros:
|
||||
|
||||
\list
|
||||
\li If the \c{QT_USE_NODISCARD_FILE_OPEN} macro is defined,
|
||||
overloads of \c{open()} are marked as \c{[[nodiscard]]}.
|
||||
\li If the \c{QT_NO_USE_NODISCARD_FILE_OPEN} is defined, the
|
||||
overloads of \c{open()} are \e{not} marked as \c{[[nodiscard]]}.
|
||||
\li If neither macro is defined, then the default up to and
|
||||
including Qt 6.9 is not to have the attribute. Starting from Qt 6.10,
|
||||
the attribute is automatically applied.
|
||||
\li If both macros are defined, the program is ill-formed.
|
||||
\endlist
|
||||
*/
|
||||
|
||||
#ifdef QT_NO_QOBJECT
|
||||
QFileDevice::QFileDevice()
|
||||
: QIODevice(*new QFileDevicePrivate)
|
||||
|
@ -12,6 +12,22 @@ QT_BEGIN_NAMESPACE
|
||||
class QDateTime;
|
||||
class QFileDevicePrivate;
|
||||
|
||||
#if !defined(QT_USE_NODISCARD_FILE_OPEN) && !defined(QT_NO_USE_NODISCARD_FILE_OPEN)
|
||||
# if QT_VERSION < QT_VERSION_CHECK(6, 10, 0)
|
||||
# define QT_NO_USE_NODISCARD_FILE_OPEN
|
||||
# else
|
||||
# define QT_USE_NODISCARD_FILE_OPEN
|
||||
# endif
|
||||
#endif
|
||||
|
||||
#if defined(QT_USE_NODISCARD_FILE_OPEN) && defined(QT_NO_USE_NODISCARD_FILE_OPEN)
|
||||
#error "Inconsistent macro definition for nodiscard QFile::open"
|
||||
#elif defined(QT_USE_NODISCARD_FILE_OPEN)
|
||||
#define QFILE_MAYBE_NODISCARD [[nodiscard]]
|
||||
#else /* QT_NO_USE_NODISCARD_FILE_OPEN */
|
||||
#define QFILE_MAYBE_NODISCARD
|
||||
#endif
|
||||
|
||||
class Q_CORE_EXPORT QFileDevice : public QIODevice
|
||||
{
|
||||
#ifndef QT_NO_QOBJECT
|
||||
|
@ -152,7 +152,7 @@ void QSaveFile::setFileName(const QString &name)
|
||||
QIODevice::ReadWrite, QIODevice::Append, QIODevice::NewOnly and
|
||||
QIODevice::ExistingOnly are not supported at the moment.
|
||||
|
||||
\sa QIODevice::OpenMode, setFileName()
|
||||
\sa QIODevice::OpenMode, setFileName(), QT_USE_NODISCARD_FILE_OPEN
|
||||
*/
|
||||
bool QSaveFile::open(OpenMode mode)
|
||||
{
|
||||
|
@ -39,7 +39,7 @@ public:
|
||||
QString fileName() const override;
|
||||
void setFileName(const QString &name);
|
||||
|
||||
bool open(OpenMode flags) override;
|
||||
QFILE_MAYBE_NODISCARD bool open(OpenMode flags) override;
|
||||
bool commit();
|
||||
|
||||
void cancelWriting();
|
||||
|
@ -725,7 +725,7 @@ QTemporaryFile::~QTemporaryFile()
|
||||
return true upon success and will set the fileName() to the unique
|
||||
filename used.
|
||||
|
||||
\sa fileName()
|
||||
\sa fileName(), QT_USE_NODISCARD_FILE_OPEN
|
||||
*/
|
||||
|
||||
/*!
|
||||
|
@ -48,7 +48,7 @@ public:
|
||||
void setAutoRemove(bool b);
|
||||
|
||||
// ### Hides open(flags)
|
||||
bool open() { return open(QIODevice::ReadWrite); }
|
||||
QFILE_MAYBE_NODISCARD bool open() { return open(QIODevice::ReadWrite); }
|
||||
|
||||
QString fileName() const override;
|
||||
QString fileTemplate() const;
|
||||
|
@ -47,6 +47,7 @@ qt_internal_add_module(DBus
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_QPAIR
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::CorePrivate
|
||||
PUBLIC_LIBRARIES
|
||||
|
@ -266,6 +266,7 @@ qt_internal_add_module(Gui
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
QT_QPA_DEFAULT_PLATFORM_NAME="${QT_QPA_DEFAULT_PLATFORM}"
|
||||
INCLUDE_DIRECTORIES
|
||||
../3rdparty/VulkanMemoryAllocator
|
||||
|
@ -62,6 +62,7 @@ qt_internal_add_module(Network
|
||||
QT_NO_CAST_TO_ASCII
|
||||
QT_NO_CAST_FROM_BYTEARRAY
|
||||
QT_NO_URL_CAST_FROM_STRING
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
kernel
|
||||
LIBRARIES
|
||||
|
@ -39,6 +39,7 @@ qt_internal_add_module(OpenGL
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::CorePrivate
|
||||
Qt::GuiPrivate
|
||||
|
@ -13,6 +13,7 @@ qt_internal_add_module(OpenGLWidgets
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::OpenGLPrivate
|
||||
Qt::WidgetsPrivate
|
||||
|
@ -23,6 +23,7 @@ qt_internal_add_module(PrintSupport
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
dialogs
|
||||
widgets
|
||||
|
@ -26,6 +26,7 @@ qt_internal_add_module(Sql
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::CorePrivate
|
||||
PUBLIC_LIBRARIES
|
||||
|
@ -65,6 +65,7 @@ qt_internal_add_module(Test
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_DATASTREAM
|
||||
QT_NO_FOREACH
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
# Ensure uniform location info between release and debug builds
|
||||
QT_NO_MESSAGELOGCONTEXT
|
||||
LIBRARIES
|
||||
|
@ -30,6 +30,7 @@ qt_internal_add_tool(${target_name}
|
||||
QT_NO_CAST_FROM_BYTEARRAY
|
||||
QT_NO_FOREACH
|
||||
QT_NO_QPAIR
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
${CMAKE_CURRENT_SOURCE_DIR}
|
||||
../../3rdparty/tinycbor/src
|
||||
|
@ -22,6 +22,7 @@ qt_internal_add_tool(${target_name}
|
||||
DEFINES
|
||||
QT_NO_FOREACH
|
||||
QT_NO_QPAIR
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::Core
|
||||
Qt::CorePrivate
|
||||
|
@ -18,6 +18,7 @@ qt_internal_add_tool(${target_name}
|
||||
QT_NO_CAST_FROM_ASCII
|
||||
QT_NO_FOREACH
|
||||
QT_RCC
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
${CMAKE_CURRENT_SOURCE_DIR}
|
||||
)
|
||||
|
@ -32,6 +32,7 @@ qt_internal_add_tool(${target_name}
|
||||
DEFINES
|
||||
QT_NO_CAST_FROM_ASCII
|
||||
QT_NO_FOREACH
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
QT_UIC
|
||||
INCLUDE_DIRECTORIES
|
||||
${CMAKE_CURRENT_SOURCE_DIR}
|
||||
|
@ -55,6 +55,7 @@ qt_internal_add_module(Widgets
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_NO_FOREACH
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
INCLUDE_DIRECTORIES
|
||||
dialogs
|
||||
LIBRARIES
|
||||
|
@ -14,6 +14,7 @@ qt_internal_add_module(Xml
|
||||
QT_NO_CONTEXTLESS_CONNECT
|
||||
QT_NO_FOREACH
|
||||
QT_NO_USING_NAMESPACE
|
||||
QT_USE_NODISCARD_FILE_OPEN
|
||||
LIBRARIES
|
||||
Qt::CorePrivate
|
||||
PUBLIC_LIBRARIES
|
||||
|
Loading…
x
Reference in New Issue
Block a user