On Fri, Apr 18, 2008 at 12:24:58AM +0200, Jakub Stachowski
wrote:
> I did some more optimizing to minimize copying data
around. Instead of using
> QByteArray everywhere I added class BufferFragment with
very similar (bare
> minimum used by parser) API, but operating on allocated
earlier big buffer.
> like left(), trim(), mid(), etc. are only pointer and
int operations.
>
kinda cool.
i have no real objections to the patch except that you
should finally
get used to placing spaces around operators. that legibility
thing,
kinda. ;)
On Fri, Apr 18, 2008 at 12:36:51AM +0200, Alexander Neundorf
wrote:
> > BufferFragment class contains very short functions
(most of them 1-3 lines)
> > that could be inlined, so all definitions are in
header file. Is it OK or
> > separate .cpp file is necessary?
>
> Not sure. Having it inline makes it faster but there
can be (binary ?)
> compatibility issues. I don't know if that matters
here, since it's just an
> internal header.
>
this doesn't make any sense. ;)
leave it in the .h.
On Fri, Apr 18, 2008 at 01:48:24AM +0200, Dirk Mueller
wrote:
> a) the readall() might kill the app if its a really big
config file. can you
> add some "chunking" so that it perhaps reads
only 128kb or something like
> that at a time, even if its a really big kconfig file?
>
i thought about that as well. but if the readAll is going to
kill the
app, then the parsed config likely will, too. given that
chunking will
make the code much more complicated and *will* introduce a
performance
hit, i'd say leave it as is.
> b) the methods do need clear documentation about the
expected state and
> calling convention, to make the patch easier to review.
>
+1
fwiw, instead of
BufferFragment &foo
for in+out parameters, you might prefer
BufferFragment *foo
- that makes it more obvious. qt uses that convention, too.
> c) please don't add an implicit QByteArray() operator..
add a toByteArray()
> function and place the conversion explicitely in the
code, so that it is
> clear when the "slow" conversion is
happening.
>
+1
--
Hi! I'm a .signature virus! Copy me into your ~/.signature,
please!
--
Confusion, chaos, panic - my work here is done.
_______________________________________________
Kde-optimize mailing list
Kde-optimize kde.org
ht
tps://mail.kde.org/mailman/listinfo/kde-optimize
|