Hi Peter, thank you for your comments! I will try to look over them and write my thoughts about them (hope I won’t screw up these quote/code tags ;)).

> rev 111
> Yes, I think you’re right, we’d better use the C++ ( instead of ).

Done (rev 116).

>rev 112
>I see you have configured your editor to remove trailing spaces and I just did the same :-)! Maybe we should also write this in the coding conventions?

That would be nice, because trailing spaces do sometimes merging problems/conflicts. I don’t know which editors can manage that, but Kate (that’s what I’m using) and Vim can.

> It’s nice that BDictionary::item() and BDictionary::keys() are const. maybe item() should return a const BItem pointer too. Otherwise it would be possible to change the contents (item) of a const dictionary.

Good remark. I’ve made BDictionary::item() and BList::item() returning const BItem * and updated all sources in Torrents/ to use const pointers when calling these functions ((rev 117)).

> Actually we don’t need the inline for the function declaration inside the class, because for the compiler it is enough to see the inline in the function definition. I read once, and I agree, that the user of the class does not need to care whether a function is inline or not.

I agree - the original reason why I’ve added inline modifier into function declarations is that I’ve seen it in other source codes. So I’ve removed them completely from the Torrent class (rev 118).
Maybe I should remove them from other headers in Protocols/BitTorrent/Torrents/?

> Following Marcus’s comment about the __ prefixed identifiers being reserved by the standard library I removed the leading __ from the include guards. Would you please do the same for your new headers?

Done (rev 118).

> Regarding the UNIT_TEST ifdef, I’m getting to believe more and more firmly that one should try to only test using the public interface …

Yes, I’m starting to having the same feeling about it since I’ve implemented some basic tests. I’ll remove it when I’ll modify Torrent unit tests.

> I think I’d actually drop the Torrent (const QByteArray &) ctor. I’m also thinking, wouldn’t it be nice to have a TorrentParser class do the parsing?

That’s a good idea! This solution with TorrentParser could be better then the current one. I’ll think about possible implementetion and I’ll inform you how it will be working.

> QSharedDataPointer does provide a copy ctor and an assignment operator for itself, so you are not required to provide these for Torrent – the compiler will do it automatically.

Ah, I didn’t know that, I’ll remove it. Maybe I should write a comment there that the Torrent class provides value semantics (copying) and that the standard implementation (done by a compiler) actually fits our needs (so we are aware of it), what do you think? (rev. 119)

> I think I’d drop the bool and ! operators. The semantics of if (torrent) may no be obvious and I’d personally prefer if (torrent == Torrent()). What do you think?

Ok, no problem, I agree that explicit test is more obvious (rev. 119). Btw what did you mean by if (torrent == Torrent())?

> We also tend to write constants in the same case as classes, i.e. HashValueSize.

Updated (rev. 120). I’ve added this constant because I’ll need it when I’ll implement indexing pieces in a QByteString (though I’ve to think up the way I’ll implement it).

Ok, that would be everything for now :). I’ll check the rest of your comments later (especially comments about unit tests) and I’ll post my remarks as I’ve done now.

Petr