rgheck wrote:
Vincent van Ravesteijn wrote:
That said, QFileInfo already caches this data. So why wasn't that
good enough? Why did Bennett have the problem that required the
refresh() calls that lead to the additional filesystem access? I
think the answer is that the first time the comparison was made, the
file didn't exist, and THAT fact got cached, so that the next time
the comparison was made, it returned false, even though the
filenames were the same. (This is the odd behavior of QFileInfo that
Vincent ran into somewhere else.) So we had to refresh. I think this
case is dealt with properly here. If we get nothing back from
canonicalFilename(), then we just compare the paths.
Yes, something like this happens indeed. Note further that the
caching of QFileInfo only makes sense if you access the same object.
In e.g. filetools.cpp:makeAbsPath() we make a new filename object
every time, which thus has a new QFileInfo object, which
automatically doesn't know about any cached values. This means that
if we keep the user defined path in e.g. InsetInclude, we create the
absolute path every time when we want to compare this value with the
one in theBufferList().
One more idea. Looking back at this thread, you once noted, Vincent,
that it's the check of QFileInfo::exists() that accesses the filesystem
in the constructor. What if we just take this out? Then QFileInfo's own
cache will deal with the makeAbsPath problem for us. Presumably we then
have a different problem, but maybe we can deal with it some other way.
But surely it's crazy to access the filesystem every time we create a
FileName object.
So, another patch, just with that change. Does this help?
Richard
Index: src/support/FileName.cpp
===================================================================
--- src/support/FileName.cpp (revision 28882)
+++ src/support/FileName.cpp (working copy)
@@ -104,11 +104,9 @@
Private() {}
Private(string const & abs_filename) : fi(toqstr(abs_filename))
- {
- fi.setCaching(fi.exists() ? true : false);
- }
+ {}
///
- inline void refresh()
+ inline void refresh()
{
// There seems to be a bug in Qt >= 4.2.0, at least, that causes problems with
// QFileInfo::refresh() on *nix. So we recreate the object in that case.
@@ -119,9 +117,16 @@
#else
fi = QFileInfo(fi.absoluteFilePath());
#endif
+ canonicalFilename_.clear();
}
+ void clone(Private rhs)
+ {
+ fi = rhs.fi;
+ canonicalFilename_ = rhs.canonicalFilename_;
+ }
+
static
bool isFilesystemEqual(QString const & lhs, QString const & rhs)
{
@@ -131,6 +136,8 @@
///
QFileInfo fi;
+ /// cache this, since it can be expensive to look it up
+ std::string canonicalFilename_;
};
/////////////////////////////////////////////////////////////////////
@@ -159,7 +166,7 @@
FileName::FileName(FileName const & rhs) : d(new Private)
{
- d->fi = rhs.d->fi;
+ d->clone(*rhs.d);
}
@@ -171,7 +178,7 @@
FileName & FileName::operator=(FileName const & rhs)
{
- d->fi = rhs.d->fi;
+ d->clone(*rhs.d);
return *this;
}
@@ -194,9 +201,30 @@
}
+string FileName::canonicalFilename() const
+{
+ if (!d->canonicalFilename_.empty())
+ return d->canonicalFilename_;
+
+ // FIXME: In future use Qt.
+ // Qt 4.4: We need to solve this warning from Qt documentation:
+ // * Long and short file names that refer to the same file on Windows are
+ // treated as if they referred to different files.
+ // This is supposed to be fixed for Qt5.
+ FileName const f(os::internal_path(absFilename()));
+
+ if (f.empty())
+ return d->canonicalFilename_; // still empty
+
+ d->canonicalFilename_ = fromqstr(f.d->fi.canonicalFilePath());
+ return d->canonicalFilename_;
+}
+
+
void FileName::set(string const & name)
{
d->fi.setFile(toqstr(name));
+ d->canonicalFilename_.clear();
}
@@ -206,12 +234,14 @@
d->fi.setFile(rhs.d->fi.filePath() + toqstr(suffix));
else
d->fi.setFile(QDir(rhs.d->fi.absoluteFilePath()), toqstr(suffix));
+ d->canonicalFilename_.clear();
}
void FileName::erase()
{
d->fi = QFileInfo();
+ d->canonicalFilename_.clear();
}
@@ -921,49 +951,13 @@
}
-// Note: According to Qt, QFileInfo::operator== is undefined when
-// both files do not exist (Qt4.5 gives true for all non-existent
-// files, while Qt4.4 compares the filenames).
-// see:
-// http://www.qtsoftware.com/developer/task-tracker/
-// index_html?id=248471&method=entry.
-bool operator==(FileName const & l, FileName const & r)
+bool operator==(FileName const & lhs, FileName const & rhs)
{
- // FIXME: In future use Qt.
- // Qt 4.4: We need to solve this warning from Qt documentation:
- // * Long and short file names that refer to the same file on Windows are
- // treated as if they referred to different files.
- // This is supposed to be fixed for Qt5.
- FileName const lhs(os::internal_path(l.absFilename()));
- FileName const rhs(os::internal_path(r.absFilename()));
-
- if (lhs.empty())
- // QFileInfo::operator==() returns false if the two QFileInfo are empty.
- return rhs.empty();
-
- if (rhs.empty())
- // Avoid unnecessary checks below.
- return false;
-
- lhs.d->refresh();
- rhs.d->refresh();
-
- if (!lhs.d->fi.isSymLink() && !rhs.d->fi.isSymLink()) {
- // Qt already checks if the filesystem is case sensitive or not.
- // see note above why the extra check with fileName is needed.
- return lhs.d->fi == rhs.d->fi
- && lhs.d->fi.fileName() == rhs.d->fi.fileName();
- }
-
- // FIXME: When/if QFileInfo support symlink comparison, remove this code.
- QFileInfo fi1(lhs.d->fi);
- if (fi1.isSymLink())
- fi1 = QFileInfo(fi1.symLinkTarget());
- QFileInfo fi2(rhs.d->fi);
- if (fi2.isSymLink())
- fi2 = QFileInfo(fi2.symLinkTarget());
- // see note above why the extra check with fileName is needed.
- return fi1 == fi2 && fi1.fileName() == fi2.fileName();
+ string lhsName = lhs.canonicalFilename();
+ string rhsName = rhs.canonicalFilename();
+ if (lhsName.empty() && rhsName.empty())
+ return lhs.absFilename() == rhs.absFilename();
+ return lhsName == rhsName;
}
Index: src/support/FileName.h
===================================================================
--- src/support/FileName.h (revision 28882)
+++ src/support/FileName.h (working copy)
@@ -64,6 +64,10 @@
/// get the absolute file name in UTF-8 encoding
std::string absFilename() const;
+ /// get the "normalized" filename, i.e., with symlinks etc resolved
+ /// returns empty if there is an error, or if the file does not exist
+ std::string canonicalFilename() const;
+
/**
* Get the file name in the encoding used by the file system.
* Only use this for accessing the file, e.g. with an fstream.
