Hi Slavcho,

Thanks for the patch! You’ve done some really neat job! I also find that lookAhead() is a pretty nice addition to BinaryReader! Here some comments and possible bugs though:

BDecoder::readInt() - I don’t really get it why you need len_pow_10. And especially the second line of:

if ((value == 0 && (num_len > 1 || sign == -1))
    || (value != 0 && value / (len_pow_10 / 10) == 0)) {

I first thought you check whether you ever get into the while loop but it you don’t (input is an ‘i’ followed by ‘e’) then value == 0 && len_pow_10 == 1 would evaluate to true. I’d expect that the second line of the above conditional would always evaluate to true.

BDecoder::readList() - on the very first line you check whether the next character is ‘l’. In your other read functions you assert this condition because readNext() must ensure it calls the correct function after “looking ahead”. I think assert is better here as well.

In this same function it might happen that item never gets initialized but item->type() will be executed. That is if an ‘e’ comes right after ‘l’. Now, I don’t know whether it is allowed to have an empty list in bencoding, but if it is not explicitly forbidden, then I think we’d better support it. The same also for empty dictionaries.

BDecoder::readDictionary() - The same as above about asserting if the next byte is not ‘d’. readNext() must guarantee the correct function is called and each function must assert that.

The same function, what happens if after reading the ‘d’ we find ourselves at the end of the file? OK, we will enter the while then readNext will be called and it will return an invalid item. I thought at first we never get out of the loop. Still, aren’t we better off with an explicit check (e.d. canRead (1))?

Still in this function, I see that value is added to the dictionary using str.value(). Seems to me you forgot to delete key.

Destructor of BDictionary - it might work a tick faster if you use Qt’s foreach and delete each entry (see dtor of BList).

I also have some concerns about using QString as key value for the dictionary (QString is Unicode). Is something said about the encoding of string values in bencoding? It might be necessary to perform some conversions of the QByteArray key before inserting items to a BDictionary.

Why don’t you make the BItem::type() a pure virtual function? You won’t need private data for BItem, but then calling type() would be slower. Well, just a suggestion… I can’t really say what is better, so use your preference. But wait, there also is a setType() function. I don’t think having this one is good!

Please check once more the docs of BDecoder::BDecoder (const QByteArray &), BList::BList()

Finally, please run the documentation through a spell checker to fix typos like “rentrun”. And please make sure all lines are maximum 80 chars in length.

It would have been nice if someone looked at my patches in that much detail as I look at yours guys. I bet some bugs would have come out! I again urge all of you to do some code review and help improve the quality of Calitko source!

Thanks!

Peter