Differential D25039 Diff 69082 src/ioslaves/ftp/ftp.cpp

Changeset View

Standalone View

View Options

src/ioslaves/ftp/ftp.cpp

Show First 20 Lines • Show All 1137 Lines • ▼ Show 20 Line(s)1131{
1138 const QString path = QString::fromLatin1(encodedPath.constData(), encodedPath.size()); 1138​ const QString path = QString::fromLatin1(encodedPath.constData(), encodedPath.size());
1139 1139​
1140 if (!ftpSendCmd((QByteArrayLiteral("mkd ") + encodedPath)) || (m_iRespType != 2)) { 1140​ if (!ftpSendCmd((QByteArrayLiteral("mkd ") + encodedPath)) || (m_iRespType != 2)) {
1141 QString currentPath(m_currentPath); 1141​ QString currentPath(m_currentPath);
1142 1142​
1143 // Check whether or not mkdir failed because 1143​ // Check whether or not mkdir failed because
1144 // the directory already exists... 1144​ // the directory already exists...
1145 if (ftpFolder(path)) { 1145​ if (ftpFolder(path)) {
1146 const QString failedPath = path; 1146​ const QString &failedPath = path;
1147 // Change the directory back to what it was... 1147​ // Change the directory back to what it was...
1148 (void) ftpFolder(currentPath); 1148​ (void) ftpFolder(currentPath);
1149 return Result::fail(ERR_DIR_ALREADY_EXIST, failedPath); 1149​ return Result::fail(ERR_DIR_ALREADY_EXIST, failedPath);
1150 } 1150​ }
1151 1151​
1152 return Result::fail(ERR_CANNOT_MKDIR, path); 1152​ return Result::fail(ERR_CANNOT_MKDIR, path);
1153 } 1153​ }
1154 1154​
▲ Show 20 Lines • Show All 213 Lines • ▼ Show 20 Line(s)1357 if (path.isEmpty() || path == QLatin1String("/")) {
1368 q->statEntry(entry); 1368​ q->statEntry(entry);
1369 return Result::pass(); 1369​ return Result::pass();
1370 } 1370​ }
1371 1371​
1372 QUrl tempurl(url); 1372​ QUrl tempurl(url);
1373 tempurl.setPath(path); // take the clean one 1373​ tempurl.setPath(path); // take the clean one
1374 QString listarg; // = tempurl.directory(QUrl::ObeyTrailingSlash); 1374​ QString listarg; // = tempurl.directory(QUrl::ObeyTrailingSlash);
1375 QString parentDir; 1375​ QString parentDir;
1376 QString filename = tempurl.fileName(); 1376​ const QString &filename = tempurl.fileName();
kossebauUnsubmitted
Done

QUrl::fileName() returns a value QString, so just

const QString filename = empurl.fileName();

While

const QString &filename = empurl.fileName();

also is fine code IIRC, as I once learned to my surprise, as the const QString & here means to tell the compiler the actual value instance should be hold only until the last use of the variable, not the end of the scope in which the variable exists, this seems also not well known by others, so might only make them confuse.
Given this is an optimization not needed here, IMHO no need to use this technique here.

kossebau: `QUrl::fileName()` returns a value QString, so just ``` const QString filename = empurl.
ahmadsamirUnsubmitted
Done

IIUC, the compiler will use a temporary object to hold the return of tempurl.fileName().

The temporary is used to initialize filename, and then it's gone:
const QString filename = tempurl.filename();

filename here is a reference-to-const to the temporary object and the temporary will have the same lifetime as the reference (for objects on the stack), so until the end of the scope:
const QString &filename = tempurl.fileName();
c.f. https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

In my mind it kind of makes more sense to use a reference-to-const when the rvalue (tempurl.fileName()) is not a temporary object, because I am saving nothing by using a reference here since the compiler will create the temporary object and hold it until the end of the scope anyway.

In this particular case, it's probably exactly the same whether the temporary is used to initialize a const non-reference object and then (the temporary) is dropped/gone or the object being initialized is a reference to const to the temporary until the end of the scope...

job->statResult() and job->url() are different because both of them return a reference to const.

ahmadsamir: IIUC, the compiler will use a temporary object to hold the return of tempurl.fileName(). The…
kossebauUnsubmitted
Done

Thanks for the reference :) to some docs about it. So my memory about this being a chance to limit scope of lifetime of that temporary object was wrong then, so it's simply the same scope as the reference variable, not limited to the last line where the variable is actually used, did I get it right from reading that? (perhaps was some limit in some compiler, it's a decade ago that I was hinted to that, by accident remember the occasion, but less the content :) ).
If so, for the rest, seems we align in not using a const ref for the variable type, instead just making it the official variable holding the object/value, not just a reference. No gain with a const reference, and just confusing by being non-standard code.

kossebau: Thanks for the reference :) to some docs about it. So my memory about this being a chance to…
1377 Q_ASSERT(!filename.isEmpty()); 1377​ Q_ASSERT(!filename.isEmpty());
1378 QString search = filename; 1378​ const QString &search = filename;
kossebauUnsubmitted
Done

This here makes the code fragile and more confusing. What is the difference between filename & search? Why is filename not const? Can it ever change? Would that be fine for search to change as well?
So while seemingly a correct optimization, as filename seems not passed to any method modifying it, the resulting code is very strange to a human reader, the intent behind to have search being a const reference to filename seems mysterious.

Without having understood the code, I would simply also make filename a const variable, and add a hint why search is a const reference only (hinting this is for optimization).
Though actually search could be possibly be removed and checked what the purpose of the asserts have been and merge this with the code upfront. But outside of scope here, so leaving just a TODO for the next person should be fine. One can still be the next person oneself :)

kossebau: This here makes the code fragile and more confusing. What is the difference between `filename`…
mevenAuthorUnsubmitted
Done

I believe search can be removed in fact, fileName can be used interchangeably.
That's the const beauty it makes this kind of issue visisble.
But this patch is about fixing atomic warnings, not fixing code around those warnings.
This can be fixed in a subsequent diff, thanks for pointing it out.

meven: I believe search can be removed in fact, fileName can be used interchangeably. That's the const…
1379 1379​
1380 // Try cwd into it, if it works it's a dir (and then we'll list the parent directory to get more info) 1380​ // Try cwd into it, if it works it's a dir (and then we'll list the parent directory to get more info)
1381 // if it doesn't work, it's a file (and then we'll use dir filename) 1381​ // if it doesn't work, it's a file (and then we'll use dir filename)
1382 bool isDir = ftpFolder(path); 1382​ bool isDir = ftpFolder(path);
1383 1383​
1384 // if we're only interested in "file or directory", we should stop here 1384​ // if we're only interested in "file or directory", we should stop here
1385 QString sDetails = q->metaData(QStringLiteral("details")); 1385​ QString sDetails = q->metaData(QStringLiteral("details"));
1386 int details = sDetails.isEmpty() ? 2 : sDetails.toInt(); 1386​ int details = sDetails.isEmpty() ? 2 : sDetails.toInt();
▲ Show 20 Lines • Show All 1357 Lines • Show Last 20 Lines