OGRE coding and style guidelines

[temas]

Thanks. I need to find some time to overhaul this, probably early next week.

[:wumpus:]

Another, perhaps trivial comment:

Use pass-by-reference over pass-by-value when possible -> but only when the type passed is larger than one int. General guideline: pass integers, pointers and oblique handles by value, but objects and structs by reference.

Although opinions differ on which is faster (one-time-copy or following an reference each time) and depend on the situation, it's clear that 4(32bit) or 8(64bit) byte constructs are very fast to push on the stack

A very difficult one, for example, is the Vector3. It only consists of three words (or 1.5 for 64 bit machines). For inline functions you really need to pass these by const reference (otherwise you will miss some nice optimialisations), but for a normal function it really depends on how many times and how the passed vector is used.

[epopov]

That may be too late and/or pointless, and even OT, but I find working with STL containers can be a pain if not typedefining a little...
A sample:

Code: Select all

std::vector<std::string> mMyContainer;
for (std::vector<std::string>::iterator it = ...)

Using typedef:

Code: Select all

typedef std::vector<std::string>           stringList;
typedef stringList::iterator               stringListIter;
typedef stringList::const_iterator         stringListCIter;
typedef stringList::reverse_iterator       stringListRIter;
typedef stringList::const_reverse_iterator stringListCRIter;

With those typedefs, you have to type less code and it looks cleaner (IMO):

Code: Select all

stringList mMyContainer;
for (stringListIter it = ...)

Using those macros, it's even easier to define a new type container and its associated iterators:

Code: Select all

#define TYPEDEF_STL_MKITERATORS(name) \
	typedef name::iterator					name##Iter; \
	typedef name::const_iterator			name##CIter; \
	typedef name::reverse_iterator			name##RIter; \
	typedef name::const_reverse_iterator	name##CRIter

#define TYPEDEF_STL_CONTAINER1(container, tp, name) \
	typedef std::container<tp>				name; \
	TYPEDEF_STL_MKITERATORS(name)

#define TYPEDEF_STL_CONTAINER2(container, tp1, tp2, name) \
	typedef std::container<tp1, tp2>		name; \
	TYPEDEF_STL_MKITERATORS(name)

#define TYPEDEF_STL_VECTOR(tp, name)		TYPEDEF_STL_CONTAINER1(vector, tp, name)
#define TYPEDEF_STL_LIST(tp, name)			TYPEDEF_STL_CONTAINER1(list, tp, name)
#define TYPEDEF_STL_MAP(tpkey, tpval, name) TYPEDEF_STL_CONTAINER2(map, tpkey, tpval, name)

You now just declare:

Code: Select all

TYPEDEF_STL_VECTOR(std::string, stringList);

and you're done.
As an added bonus, with the macros / typedefs, you can easily change the type of container (if compatibles) by changing TYPEDEF_STL_VECTOR to TYPEDEF_STL_LIST, for eg, with no other change to do (compare this with writing std::vector<std::string> everywhere: if you want to change from vector to list, you are in trouble).

My 0.1 euros.

[liberator]

I would suggest making implementation of caconical class interfaces manditory as part of the coding style.

ie always implement the default & copy constructor and the assignment operator.

This can help prevent lots of headaches caused by assumtions about classes having a propper caconical interface. If the interface of part of it isn't used then simply make them private. This way you won't be fooled by the compiler implementation of the caconical interface.

Plus I had assure the other dev's here at work that it would be implemented to get them to considder Ogre

[sinbad]

Given that the vast majority of classes should be constructed using factory classes anyway, use of copy constructors and operator= is moot in Ogre except for the internal implementations. I guess we could prevent this by defining protected copy constructors and operator= methods, but actually we do use them internally during factory construction, and C++'s lack of package visibility means that you can't prevent it externally without also preventing it internally (without 'friend' class declarations which are rather distasteful).

When you're using Ogre, you should already be in the habit of never constructing objects yourself but asking the SceneManager, ResourceManager etc to do it for you. So there are no headaches or ambiguity about what it will do, because you shouldn't do it.

[nfz]

Implementing "Canonical" class interfaces is a good practice and I have been using this style for most of my complex classes. I don't use it for all classes since the style then becomes a burden but use it where its appropiate.

[:wumpus:]

I was about to agree with liberator but the lack of package-visibility indeed makes it impossible. The only way to achieve real seperateion between interface and implementation in C++ is to use seperate outside and inside (Impl) classes.

That would be wasteful for a realtime lib like Ogre as it throws overboard any kind of inline functionality.

One could add protected assignment operators (and copy constructors) for the classes in which confusion easily arises (and for which is it possible without internal changes) though. That'd be trivial and non interface breaking, so why not?

[epopov]

One could add protected assignment operators (and copy constructors) for the classes in which confusion easily arises (and for which is it possible without internal changes) though. That'd be trivial and non interface breaking, so why not?

If I understand correctly what Sinbad said, you can't make those protected because internally one uses the assignment / copy operators of classes other than the one 'you are in': class A has a copy constructor, class B builds a new A using the copy constructor of A, but B does not inherit from A. So you must have the copy constructor of A public (or package related, something which does not exist in C++), else it won't work.

[sinbad]

Yep - although to be honest the number of uses for the operator= are fairly small, and come down often to clone() methods. Unfortunately unless you can do it for everything it becomes pointless as a 'standard' approach. Therefore the general principle remains 'thou shalt not construct Ogre objects thyself, nor shalt thou copy them or perform sinful assignment between them' .

Actually there are exceptions (aren't there always?). The 'trivial' classes such as Vector3, Matrix3, Quaternion et al are constructable and copyable because they're just custom basic types rather than Ogre objects per se. They already have all the appropriate copy constructors / operators, inlined of course.

[jwatte]

Another, perhaps trivial comment:

Although opinions differ on which is faster (one-time-copy or following an reference each time) and depend on the situation, it's clear that 4(32bit) or 8(64bit) byte constructs are very fast to push on the stack :)

Sorry to reply so late, but I don't understand how there can be a difference of opinion. Either there is a difference, and you measure it with a benchmark, or there isn't.

However, consider this: when dereferencing through a reference, you typically have the argument in a register, and use an indirect load with offset off that register. When you use an object that's constructed on the stack, you STILL use a register (the stack pointer) with an offset off that register. Thus, the reference cost is the same. However, the passing cost isn't; re-constructing the object is always more expensive than doing nothing.

Only for compilers that can pass entire objects in registers would pass-by-copy conceivably be cheaper, but none of the current ABIs actually allow this (i e, Win32, MacOS X, etc).

Regarding operator=(), the generally accepted rules seem clear: Either something is a value, in which case two separate instances that compare equal are indistinguishable and equivalent, or it's an object, in which case "equality" means "pointers are the same". If it's a value, operator=() makes sense. If it's an object, operator=() does not make sense; in fact, non-pointer instances of objects seldom make sense at all except for helpers on the stack, which may blend the two semantics at times.

Regarding "full" separation of interface and implementation, my opinion is that putting the interface as pure abstract in the header, and putting the implementation declaration in the CPP file (deriving from the interface file) works pretty well; you manufacture instances using factory functions or objects and return them by interface pointer. As far as I can tell, this achieves real separation between interface and implementation, without using pImpl. The "implementation" for copy and assignment for interface classes would be private and have no body (which would cause link error on usage).

[Attis SH]

I think the decision between call by value or call by reference is so difficult because over time more and more data will fit in registers. So it might be better to have a Vector3 passed by reference but with newer compilers, ABIs, processors etc it may change.

It would be nice to have a template like boost::call_traits<T> which has a param_type member. It could be set in OgreConfing.h for example. I understand correctly the problem is that such templates can't be used in deduced contexts so it would require a lot of work to use them consistently in all places.

[Attis SH]

Deleting pointers
I strongly suggest setting pointers to 0 or NULL after deleting them. I have macros which do delete x; x = NULL and delete [] x; x = NULL;

Generally a good idea which fits with the cause "member variables should always have a meaningful initialised value". I'd make an exception for destructors though.

I think it's especially important in destructors! I know I've lost many hours debugging code that tried to use a deleted object (before I started using smart pointers everywhere, that is). Spotting these cases is much easier if the pointers are NULL (or set to an easily recognizable pattern - 0xDEADBEEF comes to mind)

Personally I dislike setting pointers to NULL in desructors because they are simply a waste CPU cycles. This is especially so if the memory block in question is allocated just after your destructor call and filled with the contents of another class. This is why setting the pointers to NULL isn't very useful.

It may be useful to help debugging but pointers to garbage is just as "good" or easy to spot problems for me. This is why I think this practice is a bit like naming conventions: because people either hate it or like it.

A better approach would be to use the debug memory manager to fill the deallocated buffers with garbage. I think the current Ogre debug memory manager does something like this.

[Yavin]

#Control and looping statements that are followed by a "(" should have a space before the open-parenthesis.

Example 5. Parenthesis Space

if (expr)
// Space after the if
while (expr)
// Space after the while
for (expr)
// Space after the for


#Control and looping expressions inside parentheses should not have a space before or after the expression.

Example 6. Expression Spaces

if (expr) // This acceptable, no space before or after expr
if ( expr ) // UNACCEPTABLE, spaces should not be before or after expr

Why did you choose this spacing? I prefer that way:

Code: Select all

if( expr )

No space after the if statement but before and after the expression. I can read such formatted code more easy.

[haffax]

Well, this is obviously a question of taste.
Actually I can't stand

Code: Select all

if( expr )

having a whitespace between all paranthesis would unnecessarily bloat lines with many of them and having spaces only on some paranthesis would be inconsequent. So no spaces at all seems to be sensible.

Maybe one cannot argue that one is better than the other, but it is easy to understand, that the use of exactly one style makes the code more readable.

[:wumpus:]

I think the decision between call by value or call by reference is so difficult because over time more and more data will fit in registers. So it might be better to have a Vector3 passed by reference but with newer compilers, ABIs, processors etc it may change.

Bingo.
Especially classes with only inline methods could very well be stored in registers. (a vector3 or vector4 would fit nicely in one of the sse ones)
Furthermore the advanced C++ books (Strousoup, Andrescu, the C++ faq) advocate my approach, I suggest reading those to people that still think 'passing by reference is always better'.

Why did you choose this spacing? I prefer that way:

You are free to disagree, but at the end of the day, the reason why we chose one of both is not important. We just want all the code in the OGRE tree to use the same style so one was picked.

[qsilver]

Well, the way I think of it is that passing by reference is faster /now/. Any future compiler enhancements, such as passing args via SSE registers, would be designed to optimize existing code. So the most likely thing that would happen is that const references would actually become full copies, as long as the compiler finds no usage of the address-of operator.

That's my justification for passing everything by reference

As for whitespace and curly-brace/parenthesis placement, there's all sorts of style conflicts in the Ogre source. When I write patches, I just try to match the style of the neighboring code, even if it's "wrong". It'd take superhuman effort to bring all of Ogre in line with the style guidelines

[Yavin]

You are free to disagree, but at the end of the day, the reason why we chose one of both is not important. We just want all the code in the OGRE tree to use the same style so one was picked.

THX for providing me some arguments to switch my formatting

[:wumpus:]

THX for providing me some arguments to switch my formatting

Great. Another soul saved

As for whitespace and curly-brace/parenthesis placement, there's all sorts of style conflicts in the Ogre source. When I write patches, I just try to match the style of the neighboring code, even if it's "wrong". It'd take superhuman effort to bring all of Ogre in line with the style guidelines

New code should be in the new coding style, that's the point of it. We know there is a lot of old code not entirely adhering to it, but that's completely another issue.

[temas]

It will all eventually get cleaned up. Everything has just been really busy keeping this work (cleanup and review) to a minimum still.

[jwatte]

There's a lot of code that violates this requirement:

All indentation is done using 4 spaces, no tabs.

It wouldn't be all that hard to run a script over the source and fix it all up. Maybe I should prepare a patch...

[temas]

Yeah, I actually put together all the args for the indent program once. But I don't have that anymore. It is doable though, and will be redone when I get done with all this site fun.

[:wumpus:]

gnu indent messes up C++ code, badly. It's for C only really.
There are other formatting tools for C++ code though (astyle, ..)

[raicuandi]

" Each class should be contained in its won file..."

should be:

" Each class should be contained in its own file..."

Ohh... Hahaha!!! Thanks man, I was confused!

[jacmoe]

I would think that when one says 'most modern editors' they would include the most widely used editors, i.e. Microsoft editors, which do not highlight #if 0 blocks as comments. Using #if 0 is begging for disaster if you ask me. Without highlighting, delete the wrong #endif and you may introduce bugs that it will take you, or more likely, your colleagues, weeks to figure out... for no good reason.

If you need verbose comments inside of a larger function or method, the code you are commenting is probably a prime candidate for it's own function or method anyway, where you can leave the verbose comments outside.

Anyway, it's just a suggestion from personal experience. I haven't noticed much in the Ogre code that is annoying like that, but I could have choked some of the Torque developers.

Peace

I just wanted to say that VS8.0 is recognising it now - it even takes project preprocessor symbols into account, so that #IFDEF LINUX code will be grayed out because you are on Windows, and debugging code will be grayed out when in release mode, etc.
Quite nice!

[haffax]

Yes, this functionality is nice, but it still doesn't makes #ifdef 0/#endif a sensible commenting technique.