Reviewing version 2, ending with c2a5dbd.

“Fix QDir::setPath() behavior” looks good.

In “Fix “out of range” assert in QDir::operator[].“, can you explain why we shouldn’t just fix the documentation in this case? What is the use case?

In “Really use cached fileInfos when entryList(NoFilter, NoSort) was call”, the change looks wrong. updateFileLists uses data->sort and data->filters to do the sorting and filtering. The if conditions should stay where they currently are and instead updated to read
if (filters == d->data->filters && sort == d->data->sort && …)

as already happens with nameFilters. No?

I don’t think I can agree with “fix QDir::isRelative() default retval”. It changes current behavior without a rational for why absence of a fileEngine should mean relative (or not).

I guess, the concept of a relative path in Qt needs to further defined internally.

Anyway, for the time being, I suggest you drop this patch.

“QDir::cleanPath doesn’t strips trailing slash for ”\:/“ on non-win” looks good…

… as does “Clear internal entryList data immediatly after listsDirty became true.”

“remove needless clear()-s” isn’t strictly necessary, since the compiler should be able to optimize this.

Anyway, the patch looks good.

In “setPath: always init file engine”,

I would keep the initFileEngine function, making it inline. Perhaps also drop the intermediary data->fileEngine = 0;

As to the changes in setPath, simplify them to:
data->path = initFileEngine(path);

removing the calls to detach(), clear(), isRelativePath(), setFileName(), as in the current patch.

“optimize QDir::cd()” looks good.

“QDirPrivate: remove unused q_ptr and Q_DECLARE_PUBLIC” looks good.

“code cleanups and styling fixes” looks good.

Note that it includes changes that aren’t strictly necessary:
use of ternary operator instead of if/else (lines 2121-2126);
++int, instead of int++ (542, 824, 848);
use of unnamed temporaries (1692, 1738);

these are optimizations that most compilers should be able to handle by themselves. Though in the third case the difference between lvalue-ness could be significant.
using constBegin/End when the container was already const (1981).

The reason I'm pointing this is that these changes get hidden in the otherwise pure whitespace and comment changes. You could consider using separate commit when there’s enough whitespace changes.

João Abecasis

In “Fix “out of range” assert in QDir::operator[].“, can you explain why we shouldn’t just fix the documentation in this case?

done

The if conditions should stay where they currently are and instead updated to read
if (filters == d->data->filters && sort == d->data->sort && …)
as already happens with nameFilters.

done

I don’t think I can agree with “fix QDir::isRelative() default retval”

dropped

In “setPath: always init file engine”,
I would keep the initFileEngine function, making it inline.

done

“code cleanups and styling fixes” splitted into two commits

This has gone into staging/oslo-1/master and will be undergoing further QA.

I had to drop the “Fix QDir::setPath() behavior” change because it was breaking QFileSystemModel autotests, possibly others. For instance, assuming behaviour doesn’t change, should QDir constructors instead be changed so the empty path string is kept?

Anyway, impact of such changes needs more careful consideration.