Hi Peter,

I do like the trick because if the assertion ever fails we’ll also have the message printed but we in any case will also get the filename and line printed, so using that notation:

Q_ASSERT (false); // Modifying header fields of BadPackets not allowed!

will give us the same information but would require us to have a look at the source code, which we will have to do anyway. It’ll be great if we got a clean compile with no warnings. If we have plenty of them, we would get into habit of ignoring them even though they could be helpful at times. So in the sake of clean compiles I’d suggest that we the latter variant. What would you say?

I first thought that Q_ASSERT (false); would give us the same warning, but I’ve tried to test it on a simple piece of code and it does not, so yes, I agree! I’ve fix these in Protocols/BitTorrent/{Bencoding, Trackers}, so you can modify these kind of asserts in the other places (Protocols/Generics and/or Protocols/BitTorrent/Packets).

Utils/CalitkoMocks/Testing/DelayedCallDriverTest.cpp(62): warning #327: NULL reference is not allowed
fooType> driver (*(ExpectationDriverFoo *) 0, delayedFoo);
^

That’s truly hacky, so I’d better fix it :-) It does work because these tests never try to dereference the null reference. I just wanted to spare myself a few more initialization lines.

Ok ;).

./Protocols/Generics/PacketBase.h: In constructor ‘Protocols::Generics::PacketBase::Data::Data()’:
./Protocols/Generics/PacketBase.h:110: warning: ‘Protocols::Generics::PacketBase::Data::rawHeader’ should be initialized in the member initialization list
...

At that place we have not declared a default ctor and we don’t really need one. Should we define a default ctor just to satisfy the compiler? If we want “strict” code and clean compiles, then I guess we should…

I was thinking about that one few weeks ago, too, and even we don’t need it I think that we should add a default ctor - it won’t cost us anything. I’ve a default ctor in the Torrent/TrackerRequest/TrackerResponse private data classes and thus I don’t have to initialize private data in the Torent/… ctor bodies, code like this is fine:

Torrent::Torrent()
: d (new PrivateData)
{}

Torrent::PrivateData::PrivateData()
: pieceLength (0), isPrivate (false)
{}

Further advantage of this approach is that private data members are initialized only once.

[snip]
That’s actually a quite common thing, so I thought about making a macro for that. What about adding REFERENCE_OBJECT (GenericSession); in the private part of a class, which would define private copy-ctor and assignment operators (Qt have Q_DISABLE_COPY but it’s not part of the api, so we’d better define our own).

I was also thinking, in some classes we could write VALUE_OBJECT (Packet) in the public part, which would declare copy-ctor, assignment operator and the comparison operators, which would need to be defined in the cpp file. We would need to write less, but may make the class more obscure… although using the terms ValueObject and ReferenceObject might even improve readability?

Sounds like a possibly good idea, however, I’m now thinking about these things:

• What about static helpers like [Tracker,Torrent]Parser or TrackerRequestUrlCreator? These have disabled “everything” (ctor,dtor,copy ctor and assignment operator). Maybe creating another macro like STATIC_HELPER for them would be a good idea?
• As for the VALUE_OBJECT macro - we would need to implement all of the copy-ctor, assignment operator and the comparison operators, but we don’t need to define copy-ctor and assignment operator since QSharedData etc. implements these. On the other side, having them explicitely defined and commented would improve our class readability in docs (these would be present in the generated Doxygen docs), so it’s great!
  Other advantage is that we won’t ever forgot to add an operator== and operator!= (and others) definitions because linker would signal a “undefined reference…” error.
• We should document these macros using Doxygen in our documentation, but this shouldn’t be a problem. Maybe then we won’t need to document explicitely that some class “..provides value semantics (objects copying allowed) etc.”. I’m not sure how readable it will be in the generated doxygen documentation, though…

Actually these are the kind of warnings that produce probably over 90% of all calitko warnings, which means we could very soon get a clean compile :-).

Yes :).

Have a nice day,

Petr