On 29/09/2008 02:38, hzluo wrote:
Yes, the fact that this code _is not used_, provided that you use
Qt4.4. And we _will_ use Qt4.4 on Windows for 1.6.x. This code is
meant to disappear when we finally switch to 4.4, but this is not
very important. The other issue is much more important.
Yes, I know that. I can fully understand the code. But I still insist
that
the function Filename::Private::checksum() should belong to Filename
class, _not_ Filename::Private class, no matter for Qt 4.4 or 4.3
I see no reason this function must be put in Filename::Private.
Certainly I'm open for this problem. May you address your reason
to put it in Filename::Private?
My reasoning is that this is an implementation detail and I don't want
to bother others with a full recompile each time the implementation is
changed, that's all. C++ doesn't support true private implementation and
the Private class technique is often used to overcome this deficiency.
This is not a big issue anyway, I don't mind putting it in FileName if
you insist.
I have made the patch and tested. I moved your function to Filename
class and renamed to checksum_real().
As I said, those mofification are irrelevant for 1.6 on Windows. You
also moved the rest of the code outside the #ifdef for Qt which I've
put them explicitly so that it is still checked to compile even if
you use Qt4.4.
I can't understand your idea. Your original code will have _two_
methods linked in the executable if compiled with Qt 4.4
Yes, I know that this will result in dead code within the executable if
Qt4.4 is used. Nevertheless, the idea is that this code stays in
compilable state even if all developers switch to Qt4.4, because we want
to support older version on unix platforms, this is just to ease the
maintenance, that's all. AFAIK, it is also possible that the compiler
detect that this is dead code and it will not link to it.
[...]
I have tested with both Qt 4.3 and 4.4 under Windows XP.
With my patch LyX can properly handle filenames with non-ASCII chars.
I will test whether Qt 4.4 will work withour my patch.
I am moving this discussion to the development list as this needs the
input of others. As I say in bugzilla, I don't know what to do with
this patch. I suspect that we might achieve the same functionality
using iconv in a cross-platform way, but I am not sure and don't
really have the time to investigate. Anyone who knows something about
this, please chime in.
Since this is on devel list now, I may need to say something more clear.
Firstly, I think this patch should _not_ go into 1.6. It may be go into
the next version or not, we need to discuss it. But 1.6 is not suitable
as it is not a significant patch.
^^^^
You mean "it is a significant patch" I guess ;-)
OK, we agree on this point then :-)
Secondly, I strongly against the method of using mmap to compute
checksum. I suggest to use standard C file IO to read the file data with
block buffer around 64KB~16MB. mmap has different interface
on different system. It's performance is not significantly higher
than the standard file IO with proper block buffer. I see no
reason to use it at this specific situation.
You are right I guess, but our current implementation is not using a
block buffer and so is significantly slower than the mmap version (you
can check by yourself with '-dbg files'). So, in the current state, the
mmap version on Windows _is_ an improvement over the istreambuf_iterator
version. I am open to move it back to buffer based version. We might
also look at solution that doesn't calculate the checksum at all, based
on the inode on disk, or something.
Byt the way, I don't know if somebody welcomed you already on this list
but I'd like to say that I am very happy that another knowlegeable
developper joins us :-). The fact that you know about CJK is even more
interesting to us.
So welcome Hangzai Luo!
Abdel.
