Hi Peter,

I looked at the things you pointed out. I am glad you agree about the lookAhead function.

About the strange if statement in the BInt class - I know it looks strange at first.

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

The idea is to catch illegal ints such as i010e, i-0e, i00e and others. According to the specification the ints cannot be preceded by 0’s, the only valid encoding with preceding 0 is i0e, so that is why I need the len_pow_10. I will fisrt discus the second line. The statement

value / (len_pow_10 / 10)

results in the first digit of the bencoded int. So for instance if you have the given above example i010e, the value will be 10, the len_pow_10 will be 1000. 10/100 = 0 the first digit is 0 which is not allowed, therefore an error is returned. So if the value is unequal 0 and the digit after the ‘i’ is equal to 0 an error should be returned.

The first line just tests for illegal zeros like i-0e, i00e. In the specs it is nothing said about the empty encodings like ie de le. So far I accept them. I see you agree with me but what think the others. Does anyone have an idea how it must be?

About the BList. You are absolutely right. The problem with the empty list and calling item->type() exists. I changed the code so that this illegal call does not happen J . I changed also the first if with asserts in readList and readDictionary. It was thought so, but obviously I forgot it. Thanks for the hint. Now I delete the key in readDictionary too J thanks to you again. About the canRead(1) check – it is not necessary since the condition is proven by the readNext function and I think it is better to leave readNext take the responsibility there.

About the conversions of the QByteArray key it is nothing said in the specs.

I also thought of making the BItem::type() virtual but at the end decided that it is better so. Would be more writing the other way :).

I also had a look at the docs also. I hope they are ok now.

So the new patch contains everything again but with the applied corrections.

Thanks for the comments Peter. You really did a great job.

Regards Slavcho

Peter Dimov wrote:

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