QFileInfo: mark constructors as explicit

These look like leftovers (API flaws).

Construction of QFileInfo from QString (or similar) should be not
implicit, as QFileInfo construction is expensive (might hit the file
system), and this may have users overlook APIs (for instance build a
QFileInfo out of QDirIterator::next(), instead of using ::fileInfo();
using QDir::entryList instead of entryInfoList; etc.).

Leave an opt-out mechanism to ease porting.

Fix a handful of usages around qtbase, with at least a couple of them
likely to be actual "sloppy" code.

[ChangeLog][Potentially Source-Incompatible Changes][QFileInfo] Most
QFileInfo constructors are now explicit. The
QT_IMPLICIT_QFILEINFO_CONSTRUCTION macro is provided to keep old code
working.

Change-Id: Ic580e6316e67edbc840aa0c60d98c7aaabaf1af6
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
This commit is contained in:
Giuseppe D'Angelo 2020-11-12 16:11:50 +01:00 committed by Volker Hilsheimer
parent 1869615fc9
commit 784a290c4b
10 changed files with 83 additions and 17 deletions

View File

@ -335,11 +335,11 @@ SubdirsMetaMakefileGenerator::init()
QFileInfo subdir(subdirs.at(i).toQString()); QFileInfo subdir(subdirs.at(i).toQString());
const ProKey fkey(subdirs.at(i) + ".file"); const ProKey fkey(subdirs.at(i) + ".file");
if (!project->isEmpty(fkey)) { if (!project->isEmpty(fkey)) {
subdir = project->first(fkey).toQString(); subdir = QFileInfo(project->first(fkey).toQString());
} else { } else {
const ProKey skey(subdirs.at(i) + ".subdir"); const ProKey skey(subdirs.at(i) + ".subdir");
if (!project->isEmpty(skey)) if (!project->isEmpty(skey))
subdir = project->first(skey).toQString(); subdir = QFileInfo(project->first(skey).toQString());
} }
QString sub_name; QString sub_name;
if(subdir.isDir()) if(subdir.isDir())

View File

@ -597,8 +597,8 @@ QString qmake_abslocation();
static QString getPrefixFromHostBinDir(const char *hostBinDirToPrefixPath) static QString getPrefixFromHostBinDir(const char *hostBinDirToPrefixPath)
{ {
const QFileInfo qmfi = QFileInfo(qmake_abslocation()).canonicalFilePath(); const QString canonicalQMakePath = QFileInfo(qmake_abslocation()).canonicalPath();
return QDir::cleanPath(qmfi.absolutePath() + QLatin1Char('/') return QDir::cleanPath(canonicalQMakePath + QLatin1Char('/')
+ QLatin1String(hostBinDirToPrefixPath)); + QLatin1String(hostBinDirToPrefixPath));
} }

View File

@ -1621,5 +1621,62 @@ QDebug operator<<(QDebug dbg, const QFileInfo &fi)
Returns symLinkTarget() as a \c{std::filesystem::path}. Returns symLinkTarget() as a \c{std::filesystem::path}.
\sa symLinkTarget() \sa symLinkTarget()
*/ */
/*!
\macro QT_IMPLICIT_QFILEINFO_CONSTRUCTION
\since 6.0
\relates QFileInfo
Defining this macro makes most QFileInfo constructors implicit
instead of explicit. Since construction of QFileInfo objects is
expensive, one should avoid accidentally creating them, especially
if cheaper alternatives exist. For instance:
\badcode
QDirIterator it(dir);
while (it.hasNext()) {
// Implicit conversion from QString (returned by it.next()):
// may create unnecessary data strucutres and cause additional
// accesses to the file system. Unless this macro is defined,
// this line does not compile.
QFileInfo fi = it.next();
~~~
}
\endcode
Instead, use the right API:
\code
QDirIterator it(dir);
while (it.hasNext()) {
it.next();
// Extract the QFileInfo from the iterator directly:
QFileInfo fi = it.fileInfo();
~~~
}
\endcode
Construction from QString, QFile, and so on is always possible by
using direct initialization instead of copy initialization:
\code
QFileInfo fi1 = some_string; // Does not compile unless this macro is defined
QFileInfo fi2(some_string); // OK
QFileInfo fi3{some_string}; // Possibly better, avoids the risk of the Most Vexing Parse
auto fi4 = QFileInfo(some_string); // OK
\endcode
This macro is provided for compatibility reason. Its usage is not
recommended in new code.
*/
QT_END_NAMESPACE QT_END_NAMESPACE

View File

@ -59,23 +59,32 @@ class Q_CORE_EXPORT QFileInfo
public: public:
explicit QFileInfo(QFileInfoPrivate *d); explicit QFileInfo(QFileInfoPrivate *d);
#ifdef QT_IMPLICIT_QFILEINFO_CONSTRUCTION
#define QFILEINFO_MAYBE_EXPLICIT Q_IMPLICIT
#else
#define QFILEINFO_MAYBE_EXPLICIT explicit
#endif
QFileInfo(); QFileInfo();
QFileInfo(const QString &file); QFILEINFO_MAYBE_EXPLICIT QFileInfo(const QString &file);
QFileInfo(const QFileDevice &file); QFILEINFO_MAYBE_EXPLICIT QFileInfo(const QFileDevice &file);
QFileInfo(const QDir &dir, const QString &file); QFILEINFO_MAYBE_EXPLICIT QFileInfo(const QDir &dir, const QString &file);
QFileInfo(const QFileInfo &fileinfo); QFileInfo(const QFileInfo &fileinfo);
#ifdef Q_CLANG_QDOC #ifdef Q_CLANG_QDOC
QFileInfo(const std::filesystem::path &file); QFileInfo(const std::filesystem::path &file);
QFileInfo(const QDir &dir, const std::filesystem::path &file); QFileInfo(const QDir &dir, const std::filesystem::path &file);
#elif QT_CONFIG(cxx17_filesystem) #elif QT_CONFIG(cxx17_filesystem)
template<typename T, QtPrivate::ForceFilesystemPath<T> = 0> template<typename T, QtPrivate::ForceFilesystemPath<T> = 0>
QFileInfo(const T &file) : QFileInfo(QtPrivate::fromFilesystemPath(file)) { } QFILEINFO_MAYBE_EXPLICIT QFileInfo(const T &file) : QFileInfo(QtPrivate::fromFilesystemPath(file)) { }
template<typename T, QtPrivate::ForceFilesystemPath<T> = 0> template<typename T, QtPrivate::ForceFilesystemPath<T> = 0>
QFileInfo(const QDir &dir, const T &file) : QFileInfo(dir, QtPrivate::fromFilesystemPath(file)) QFILEINFO_MAYBE_EXPLICIT QFileInfo(const QDir &dir, const T &file) : QFileInfo(dir, QtPrivate::fromFilesystemPath(file))
{ {
} }
#endif // QT_CONFIG(cxx17_filesystem) #endif // QT_CONFIG(cxx17_filesystem)
#undef QFILEINFO_MAYBE_EXPLICIT
~QFileInfo(); ~QFileInfo();
QFileInfo &operator=(const QFileInfo &fileinfo); QFileInfo &operator=(const QFileInfo &fileinfo);

View File

@ -350,8 +350,7 @@ QExtendedInformation QFileInfoGatherer::getInfo(const QFileInfo &fileInfo) const
#ifdef Q_OS_WIN #ifdef Q_OS_WIN
if (m_resolveSymlinks && info.isSymLink(/* ignoreNtfsSymLinks = */ true)) { if (m_resolveSymlinks && info.isSymLink(/* ignoreNtfsSymLinks = */ true)) {
QFileInfo resolvedInfo(fileInfo.symLinkTarget()); QFileInfo resolvedInfo(QFileInfo(fileInfo.symLinkTarget()).canonicalFilePath());
resolvedInfo = resolvedInfo.canonicalFilePath();
if (resolvedInfo.exists()) { if (resolvedInfo.exists()) {
emit nameResolved(fileInfo.filePath(), resolvedInfo.fileName()); emit nameResolved(fileInfo.filePath(), resolvedInfo.fileName());
} }

View File

@ -230,7 +230,7 @@ static void sm_performSaveYourself(QXcbSessionManager *sm)
restart << argument0 << QLatin1String("-session") restart << argument0 << QLatin1String("-session")
<< sm->sessionId() + QLatin1Char('_') + sm->sessionKey(); << sm->sessionId() + QLatin1Char('_') + sm->sessionKey();
QFileInfo fi = QCoreApplication::applicationFilePath(); QFileInfo fi(QCoreApplication::applicationFilePath());
if (qAppName().compare(fi.fileName(), Qt::CaseInsensitive) != 0) if (qAppName().compare(fi.fileName(), Qt::CaseInsensitive) != 0)
restart << QLatin1String("-name") << qAppName(); restart << QLatin1String("-name") << qAppName();
sm->setRestartCommand(restart); sm->setRestartCommand(restart);

View File

@ -2211,7 +2211,7 @@ QString QTest::qFindTestData(const QString& base, const char *file, int line, co
// 3. relative to test source. // 3. relative to test source.
if (found.isEmpty() && qstrncmp(file, ":/", 2) != 0) { if (found.isEmpty() && qstrncmp(file, ":/", 2) != 0) {
// srcdir is the directory containing the calling source file. // srcdir is the directory containing the calling source file.
QFileInfo srcdir = QFileInfo(QFile::decodeName(file)).path(); QFileInfo srcdir(QFileInfo(QFile::decodeName(file)).path());
// If the srcdir is relative, that means it is relative to the current working // If the srcdir is relative, that means it is relative to the current working
// directory of the compiler at compile time, which should be passed in as `builddir'. // directory of the compiler at compile time, which should be passed in as `builddir'.

View File

@ -1561,8 +1561,9 @@ QList<QtDependency> findFilesRecursively(const Options &options, const QFileInfo
const QStringList entries = dir.entryList(QDir::Files | QDir::Dirs | QDir::NoDotAndDotDot); const QStringList entries = dir.entryList(QDir::Files | QDir::Dirs | QDir::NoDotAndDotDot);
for (const QString &entry : entries) { for (const QString &entry : entries) {
QString s = info.absoluteFilePath() + QLatin1Char('/') + entry; ret += findFilesRecursively(options,
ret += findFilesRecursively(options, s, rootPath); QFileInfo(info.absoluteFilePath() + QChar(u'/') + entry),
rootPath);
} }
return ret; return ret;

View File

@ -364,7 +364,7 @@ static inline QFileInfo findSh()
const QStringList rawPaths = QString::fromLocal8Bit(pEnv.constData()).split(pathSep, Qt::SkipEmptyParts); const QStringList rawPaths = QString::fromLocal8Bit(pEnv.constData()).split(pathSep, Qt::SkipEmptyParts);
foreach (const QString &path, rawPaths) { foreach (const QString &path, rawPaths) {
if (QFile::exists(path + sh)) if (QFile::exists(path + sh))
return path + sh; return QFileInfo(path + sh);
} }
return QFileInfo(); return QFileInfo();
} }

View File

@ -126,7 +126,7 @@ void tst_QFileIconProvider::type()
static QIcon getIcon() static QIcon getIcon()
{ {
QFileIconProvider fip; QFileIconProvider fip;
return fip.icon(QDir::currentPath()); return fip.icon(QFileInfo(QDir::currentPath()));
} }
void tst_QFileIconProvider::taskQTBUG_46755_QFileIconEngine_crash() void tst_QFileIconProvider::taskQTBUG_46755_QFileIconEngine_crash()