Source and Buildsystem

Everything about development and the OpenMW source code.
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Source and Buildsystem

Post by lgromanowski »

Zini wrote: After looking seriously at the code for the first time, I have a couple of comments to make:

1. The source uses both .h and .hpp extensions. Either one is good. Having both at the same time is not. Actually it is bloody annoying.

2. The cmake script lists source files individually. While I am aware, that Ogre is doing the same, it looks to me like a lot of unnecessary work. Can't you just GLOB the files?

3. The cmake script doesn't list header files, which is problematic when generating project files for IDEs.

4. There are a lot of const char * in the code. While it is okay to use const char * for a string literal, the use of C-strings in C++ code is pretty much universally frowned upon.

5. You are including Ogre.h in some of your files. This is a very bad idea, since it slows down the compilation (as you have noticed yourself). Better only include those Ogre headers, that you need.
nicolay wrote:
Zini wrote:After looking seriously at the code for the first time, I have a couple of comments to make:

1. The source uses both .h and .hpp extensions. Either one is good. Having both at the same time is not. Actually it is bloody annoying.
I agree that it's inconsistent. As you probably noticed it was fixed a few days ago, I renamed all .h to .hpp.

(The reason I switched in the first place was because emacs started misbehaving on syntax highlighting C++ in .h files. Emacs is an example of software that seems to be evolving backwards for each new release.)
Zini wrote:2. The cmake script lists source files individually. While I am aware, that Ogre is doing the same, it looks to me like a lot of unnecessary work. Can't you just GLOB the files?
Not a big fan of wildcards in build scripts. I very often have unfinished files laying around and I don't want CMake to trip up trying to build them. But I understand the sentiment.
Zini wrote:3. The cmake script doesn't list header files, which is problematic when generating project files for IDEs.
That's a great idea. I have no idea how to do this - would you be willing to help out / submit a patch for this?
Zini wrote:4. There are a lot of const char * in the code. While it is okay to use const char * for a string literal, the use of C-strings in C++ code is pretty much universally frowned upon.
Yes sorry about that, that's my compulsive optimization running amok. I don't mind const char* used internally in functions but there are probably many places we should substitute std::string in function parameters.
Zini wrote:5. You are including Ogre.h in some of your files. This is a very bad idea, since it slows down the compilation (as you have noticed yourself). Better only include those Ogre headers, that you need.
That's true. I'll change it when it bothers me enough (or I'll accept patches before that :))
Zini wrote: I'll take care of the header problem. Regarding the GLOBing, I understand your position. So we will stick with the individual file listing.
Locked