Libzypp/Devel/Coding
İçindekiler
Coding rules
(Based on comments given by Michael Andres)
File header
Use this at the beginning of a file.
- A doxygen comment should be present in each file.
- \file should be followed by the source files path from zypp/ on.
- The same path (all uppercase and '/' replaced by '_') as define against multiple inclusion in headers.
- If there is a description of the files content, put it below \file.
NOTE: Zypp's doxygen configuration has AUTOBRIEF turned on. Whatever comment you write (file, class, function, ..), everything up to the 1st '.' or empty line (whichever appears first) is used as \brief description. So you can write as well:
/** \file zypp/parser/YUM/YUMParserData.h * * Declares the various YUMData classes. They are rather dumb * structure-like classes that hold the data ....
But the first sentence should be descriptive.
Don't #include<iostream> in header file
Do your best to NOT include <iostream> (fstream, etc.) in your header file. It's a quite exepensive include. Instead of it:
#include<iosfwd>
Namespaces
DON'T do this
namespace zypp { namespace foo { namespace bar {
...
/* end of namespace bar */
</pre>
But write it as separate blocks and indent. Either
namespace zypp {
namespace foo {
namespace bar {
...
}
}
}
or
namespace zypp
{
namespace foo
{
namespace bar
{
...
}
}
}
Never use using in a header!
Unless it does what you actually want it to do :)
e.G.
using namespace std; // do not
Why? Because the using does not end at the end of the header file. It's applied to everything that includes your file. 'using namespace std' introduces new names (the ones from ::std) wherever it is included, and thus may create clashes and ambiguities.
Please take it serious. We get into deep trouble, if we mess up the namespaces.
Classes
Inline constructors
class MetaPkg {
public:
MetaPkg();
MetaPkg(const std::string& type,
const std::string& name);
std::string type;
std::string name;
};
Like almost all classes, they are actually structs. The constructors are just a convenience to initialize the data. So you can inline them if they are trivial.
MetaPkg() {}
MetaPkg(const std::string& type,
const std::string& name)
: type(type), name(name)
{}
Reference counter
class YUMPatchPackage : public base::ReferenceCounted, private base::NonCopyable {
public:
YUMPatchPackage() {};
// data for primary
std::string type;
std::string name;
...
};
- public base::ReferenceCounted
Im not sure whether it actually makes sense to spend an intrusive reference counter for structs like this. They are created, used and thrown away. There are probably not that much pointer assignments as we'll have for e.g. Resolvables. So there's probably not much performance penalty, but we can save the baseclass, the vtable (ReferenceCounted has a virtual dtor) and the instanciated refcount handling template functions for these types.
One should perhaps use zypp::base::shared_ptr as smart pointer. For these structs. It's just a different notation, but they behave like the YUMPatchPackagePtr.
shared_ptr<YUMPatchPackage> yptr( new YUMPatchPackage );
NOTE: Nice about shared_ptr is the collaboration with weak_ptr. A weak_ptr can be constructed from a shared_ptr, and you can later retrieve a shared_ptr back from the weak_ptr. But, the weak_ptr does not hold a reference to the object. Thus it does not prevent the object from being destucted, but in case it is, the weak pointer 'knows' that the Object is no longer available.
- private base::NonCopyable
Why? Well, CopyCtor or assign isn't that cheap on lage structs, but neither too expensive nor unsafe. So why not? And as you probably operate with smart pointers most of the time, there won't be much copy or assign at all.
Unions vs. common base classes
class YUMPatchAtom {
public:
std::string type;
// union not possibel due to constructors
// there is no common parent
YUMPatchPackagePtr package;
YUMPatchScriptPtr script;
YUMPatchMessagePtr message;
};
A union? This is not C ;) 'there is no common parent', so we shouLd create one!
And some structs later..
// FIXME this copying of structures is inefective std::list<YUMPatchAtom> atoms;
Thus different YUMPatchAtom are to be created and shipped in a list?
---[draft]---
struct YUMAtom {
virtual ~YUMAtom() {}
// A virtual dtor is needed if we need RTTI to retrieve the
// original type of a YUMAtom stored in a list
};
struct YUMPatchPackage : public YUMAtom { /*body*/ };
struct YUMPatchScript : public YUMAtom { /*body*/ };
struct YUMPatchMessage : public YUMAtom { /*body*/ };
Now there's a common base class! Draft how to use it:
void printAtom( shared_ptr<YUMAtom> a ); // forward decl.
int main()
{
list<shared_ptr<YUMAtom> > lst; // global list to hold atoms
// create real atom in parser and append it to global list
shared_ptr<YUMPatchPackage> pAtomPtr( new YUMPatchPackage );
// fill YUMPatchPackage...
lst.push_back( pAtomPtr );
// another type of atom
shared_ptr<YUMPatchScript> sAtomPtr( new YUMPatchScript );
lst.push_back( sAtomPtr );
// and another one
shared_ptr<YUMPatchMessage> mAtomPtr( new YUMPatchMessage );
lst.push_back( mAtomPtr );
// print out atoms in list... see printAtom
for_each( lst.begin(), lst.end(), printAtom );
}
void printAtom( shared_ptr<YUMAtom> a );
{
cerr
<< dynamic_pointer_cast<YUMPatchPackage>(a) << ' '
<< dynamic_pointer_cast<YUMPatchScript>(a) << ' '
<< dynamic_pointer_cast<YUMPatchMessage>(a) << endl;
// So if (a!=0) then dynamic_pointer_cast<concrete atom type>
// is != 0, iff atom 'a' is of type 'concrete atom type'
//
// To recover the concrete types we'll need some
// if ( dynamic_pointer_cast<YUMPatchPackage>(a) != 0 )
// ...
// else if ( dynamic_pointer_cast<YUMPatchScript>(a) )
// ...
// cascade.
}
Dynamic casts
Dynamic casts are expensive. We could as well consider to introduce an {{{enum Type { Package, Script, Message</pre>} in struct YUMAtom. And some pure virtual method virtual YUMAtom::Type type() const = 0; in YUMAtom.
Then we could do (in something like printAtom):
switch ( a->type() )
{
case Package:
dynamic_pointer_cast<YUMPatchPackage>(a)
...
case Script:
dynamic_pointer_cast<YUMPatchScript>(a)
...
case Message:
dynamic_pointer_cast<YUMPatchMessage>(a)
...
}
Probably not faster, but the compiler warns if enum values are not handled in switch.
(I know I'm picky)
std::ostream& operator<<
In YUMParserData.h they are declared outside the zypp::parser::YUM namespace. They should be declared in the same namespace as the class they print.
König lookup makes the compiler lookup functions in the namespace of it's arguments:
cout << zypp::parser::YUM::FileData();
Looking for operator<<, g++ will consider namespace zypp::parser::YUM to find operator<<. If it's dragged into the global namespace, it might interfere with some operator<< for some foo::baa:::FileData().
The case is not easy to construct, but we can avoid it.
Do not inline 'std::ostream operator<<'
unless it's trivial. As you don't include <iostream>, you actually can't output any integal type nor use std::endl.
inline std::ostream & operator<<( std::ostream & out,
const ChangelogEntry & obj )
{
out << obj.date() << " " << obj.author() << endl << obj.text() << endl;
return out;
}
This is no longer possible, but that's IMO ok as you don't want to do any kind of formating in the header.
What you can do is calling operator<< of some other known class, if you're quite shure it does not make sense to change this in the future:
class Path
{
friend std::ostream & operator<<( std::ostream & str, const Path & obj );
public:
const std::string asString() const;
protected:
virtual std::ostream & dumpOn( std::ostream & str ) const;
private:
std::string _path;
};
inline std::ostream & operator<<( std::ostream & str, const Path & obj )
{
return str << obj.asString();
// if <string> is included; operator<< for std::string is declared, and
// <iosfwd> is sufficient to write this.
}
//or
{
return obj.dumpOn( str );
// That's a sometimes handy way for hirearchies. operator<< is needed for
// the base class only. Derived classes simply overload dumpOn, if they
// nedd something different, or append details to the base opertor.
}
Doxygen for related functions Please tag operators or related functions declared outside a class:
/** \relates ChangelogEntry */ std::ostream & operator<<( std::ostream & out, const ChangelogEntry & obj );
You can of course add further comments if you like. But \relates <class> makes doxygen list these function in the class' documentation. So you immediately see what additional operators or related functions are available for this class.
Last edit in Trac '12/05/05 14:14:12' by 'schubi'
Last edit in Trac '12/05/05 14:14:12' by 'schubi'
Last edit in Trac '12/05/05 14:14:12' by 'schubi'
Last edit in Trac '12/05/05 14:14:12' by 'schubi'