In “fix regression introduced in 44766d2” do you have a test case? When do file engines return empty/null fileNames?

Also note that we’ll be returning different values the first time this function is called and on subsequent calls (null vs. empty). Is this difference important?

in “don’t call clear() from constructors”, do you know where QFileInfoPrivate::Data’s copy constructor is being called, if anywhere? Could it go away entirely or turned to a private declaration with no implementation?

Why are you no longer copying the value of fileSize? Should you instead be copying that value together with the value of CachedFlags? Any other flags that could be copied from the source?

In “Don’t re-stat files in caching mode” lines 159-160 seem unnecessary. Other than that, the patch looks good and can be accepted.

I leave a few questions here for bonus points ;–)

Would it be possible to lump together all calls to fileEngine->fileFlags and call it only once, e.g., if FileInfoAll is requested?

Also, why are we arbitrarily asking for FileInfoAll | LocalDiskFlag (FlagsMask | TypesMask with your patch) on all requests, shouldn’t we leave this optimization up to the file engine to decide?

Could the function be reduced to a single check on whether cache is enabled; a call to the engine with the user request and an update of the cached flags?

“Simplify QFileInfoPrivate::getFileTime” looks good.

As does “move includes into proper place”. Although we could argue on the definition of “proper place” :-p

Ditto for “implement additional caching of FileOwner strings” and “cleanups & styling fixes”. They look good.

João Abecasis

In “fix regression introduced in 44766d2” do you have a test case? When do file engines return empty/null fileNames?

CanonicalName / CanonicalPathName + !file.exists(), LinkName + !file.isSymlink and so on

in “don’t call clear() from constructors”, do you know where QFileInfoPrivate::Data’s copy constructor is being called, if anywhere? Could it go away entirely or turned to a private declaration with no implementation?

qAtomicDetach (detach(), setCaching(), initFileEngine())

Why are you no longer copying the value of fileSize? Should you instead be copying that value together with the value of CachedFlags? Any other flags that could be copied from the source?

there is no sense to do that since all cached data becomes invalid anyways

Would it be possible to lump together all calls to fileEngine->fileFlags and call it only once, e.g., if FileInfoAll is requested?
Also, why are we arbitrarily asking for FileInfoAll | LocalDiskFlag (FlagsMask | TypesMask with your patch) on all requests, shouldn’t we leave this optimization up to the file engine to decide?
Could the function be reduced to a single check on whether cache is enabled; a call to the engine with the user request and an update of the cached flags?

done

Merged to master branch as of 028b4b5. Also take notice of fix in 2877e35.