🔒 Locked

Simple Inheritance Question

Started by toro Sep 8, 2005 at 1:43 AM 13 replies 1.5k views

Original Post

Sep 8, 2005 at 1:43 AM

This is my first post on this forum, so please be patient with me...:D I got this working code: /* ctask.h */


 class ctask
 {
     public:
     ctask();
     virtual ~ctask();
     virtual void DoSomething();
 };

/* ccomp.h */


 class ccomp: public ctask
 {
     public:
     ccomp();
     ~ccomp();     
     void DoSomething();
     /*... other methods ...*/
 };

 void ccomp::DoSomething()
 {
     /* do something..*/
 }

/* ckernel.h */


 #include <ctask.h>
 #include <ccomp.h>

 class ckernel
 {
      public:
      ctask *pctask[21];
      int    task_count;

      ckernel();
      ~ckernel();
      void Run();
      void AddTask(ctask *ptask);
 }

 ckernel::ckernel()
 {
      AddTask((ctask *) new ccomp);
      AddTask((ctask *) new ccomp);
 }

 ckernel::~ckernel()
 {
      for (int i=0; i<task_count;i++)
      {
           delete (pctask);
      }
 }

 void ckernel::Run()
 {
      for (int i=0; i<task_count;i++)
      {
           pctask->DoSomething(); 
      }
 }
  
 void ckernel::AddTask (ctask *ptask)
 {
      if (task_count < 21)
      {  
           pctask[task_count++] = ptask;    
      }
      else
      {
           /* do nothing */
      }
 }

I'm pretty new working with C++, so I want to ask some questions about this implementation: 1. I found out that I cannot include ctask.h file in ccomp.h file, because ctask.h is already included in ckernel.h therefore the ctask class is already defined when ccomp.h is compiled. From my previous C experience, I know how to implement an #ifdef CTASK_DEFINED mechanism to avoid this double declaration of ctask class, but I want to know if there is another way to avoid this [a cleaner solution without #ifdef]. 2. "(ctask *) new ccomp" is acceptable? 3. What do you think about the way this "ckernel" class is implemented, do you have any suggestions? 4. The coding style is acceptable? Thank you, [Edited by - toro on September 8, 2005 6:26:28 AM]

Peregrin

Sep 8, 2005 at 2:01 AM

The solution to your first issue is using include guards. For example:

/// This goes in ctask.h:#ifndef _CTASK_H_#define _CTASK_H_// The rest of the header's contents goes here#endif

Note that the name name you define, _CTASK_H_ in the example, is not really important as long as it is unique to that header file.

As for the rest of your question, please edit your original message and put the code inside source or code tags. This will make your code much more readable.

stylin

Sep 8, 2005 at 2:31 AM

Quote:
Original post by toro
3. What do you think about the way this "ckernel" class is implemented, do you have any suggestions?

Since we're coding in c plus plus, I'd recommend storing your tasks in a vector or list.

Quote:
2. "(ctask *) new ccomp" is acceptable?

I would do ctask( ccomp ). (create the task beforehand, then pass a reference to the kernel)

Quote:
4. The coding style is acceptable?

Besides the above, implementation seems OK. As for the format, I can't tell. You can use the source tags described here. Besides that, you may want to look at these articles. They describe a Kernel system similar to yours.

toro

Sep 8, 2005 at 4:10 AM

thanks for answers (i didn't know about source tag)

Peregrin wrote:

Quote:
The solution to your first issue is using include guards

I do know the mechanism, however I didn't know that it is called guards... my question is if it is common to C++ implementations or there is any reason why I should avoid it?

stylin wrote:

Quote:
Since we're coding in c plus plus, I'd recommend storing your tasks in a vector or list.

probably the code is not complete, which means that I would like to add something like this:
/*ccomp2.h*/

class ccomp2: public ctask {     public:     ccomp2();     ~ccomp2();          void DoSomething();     /*... other methods than ccomp class...*/ }; void ccomp2::DoSomething() {     /* do something..*/ }

so in ckernel.h I would update the cosntructor with this:

 ckernel::ckernel() {      AddTask((ctask *) new ccomp);      AddTask((ctask *) new ccomp2); }

In this scenario, is still possible to use vector class?

stylin wrote:

Quote:
I would do ctask( ccomp ). (create the task beforehand, then pass a reference to the kernel)

Sorry, I'm not familiar with this, but I will look into it. Thanks for pointing out that article.

[Edited by - toro on September 8, 2005 6:10:59 AM]

nmi

Sep 8, 2005 at 4:54 AM

Quote:
Original post by toro
I'm pretty new working with C++, so I want to ask some questions about this implementation:
1. I found out that I cannot include ctask.h file in ccomp.h file, because ctask.h is already included in ckernel.h therefore the ctask class is already defined when ccomp.h is compiled.
From my previous C experience, I know how to implement an #ifdef CTASK_DEFINED mechanism to avoid this double declaration of ctask class, but I want to know if there is another way to avoid this [a cleaner solution without #ifdef].

The #ifdef solution is fine. Since inclusion guards protect multiple inclusion of a file, it should be the filename that is part of the defined name, not just a class in this file.

Quote:

2. "(ctask *) new ccomp" is acceptable?

Why not just "new ccomp" without a cast ? The compiler will downcast automatically.

Quote:

3. What do you think about the way this "ckernel" class is implemented, do you have any suggestions?

Maybe you should not implement the class in the header, but in another file, i.e. ckernel.cc
Usually only template classes or very small functions of a class are implemented in the header. If you implement small member functions in the class, they are inline by default, which may even reduce code size since there is no need to push the parameters on the stack and call the functions.
Another thing are the #include directives. For header files that are part of your project, you should use #include "ctask.h", for header files of the (standard) library, you should use #include .
Also define a constant if you use it multiple times.
Example:

class ckernel{        enum { MAX_TASK_COUNT = 21 };    public:        ctask* pctask[MAX_TASK_COUNT];};

Quote:

4. The coding style is acceptable?

Having a style is a good thing. It makes your program easier to read, even if others are not familar with your style. Also comments are a great help. Check out tools like doxygen, that allow you to create a browseable documentation for your class hierarchy. Also grouping statements into paragraphs (i.e. three statements that belong together, then an empty line) allows to follow the code more easily.
Also some spaces would makes things easier to read:

for (int i=0; i<task_count;i++)

becomes

for (int i = 0; i < task_count; i++)

When using a type, write all of what belongs to the type together:

ctask *pctask[21];

becomes

ctask* pctask[21];

stylin

Sep 8, 2005 at 5:39 AM

Quote:
Original post by toro
I do know the mechanism, however I didn't know that it is called guards... my question is if it is common to C++ implementations or there is any reason why I should avoid it?

Not at all. It's the right thing to do. (OPS: plus pluses are getting chopped)

Your ckernel object is responsible for running tasks and killing tasks. In main (probably) instantiate your ckernel, then your tasks. Then add your tasks one by one. Right now you're prioritizing by chronological order (higher priority tasks are added first). If you store your tasks in a vector or list, it's much easier to define priority when adding tasks. In addition, killing, re-prioritizing or suspending tasks are made much simpler, since re-ordering is done for you by the STL. The set of articles I linked to explain this (task prioritization and killing) in great detail, although if you're not familiar with smart pointers, you may want to research those.

Quote:

   ctask *pctask[21];   int task_count;

You're coding in C plus plus now, time to drop those unsafe, pain in the butt C arrays[smile]. Also, you're limiting your task count - why?

Quote:

void ckernel::Run() {      for (int i=0; i<task_count;i++)      {           pctask->DoSomething();       } }

Here you're iterating through your task array, and executing them all. Then what? What if a task wants to die? What if there are no more tasks? Currently you have no support for killing tasks without alot of hassle - this is why you use a std::list. Instead of having seperate functions in your kernel, you can do all of this in this function.

As long as you have living tasks, iterate through your task list, and DoSomething() to all tasks that are not ready to die. Iterate again and remove those tasks that are (if you're using smart pointers this effectively destroys the task for you).

So, ckernel::Run executes until there are no more tasks. (What else would you want to do inbetween your task loop? Remember, your tasks are the objects that make up your system, the kernel just sees that they get executed.)

style-wise, I can read your code well. And if you have nobody else that's looking at your code, (or going to be looking at it) then all that matters is if you can read it. I do the following:

class SomeClass {   int some_int_;   int * psome_int_;public:   SomeClass( int some_var ):      some_int_(some_var), psome_int_( new int( some_int_ )) {};  ~SomeClass() { delete psome_int_; };};

RAZORUNREAL

Sep 8, 2005 at 6:20 AM

I'm not sure why no-one has mentioned this, but the

else{     /* do nothing */}

is entirely pointless. In fact I'm pretty sure you can't put an else after a for loop.

toro

Sep 8, 2005 at 6:35 AM

RAZORUNREAL wrote:

Quote:
I'm not sure why no-one has mentioned this, but the

else
{
/* do nothing */
}

is entirely pointless. In fact I'm pretty sure you can't put an else after a for loop.

You are right about the else {} after for...it was a typo.(corrected)

I put the else {/* do nothing */} because if anybody looks in my code, he will know that I didn't ignore the else branch and it can be sure that nothing should be executed on that branch. But other than this is pretty pointless...

toro

Sep 8, 2005 at 7:50 AM

nmi, stylin thanks for ur great replies, however I need some time to "assimilate" this new knowledge...:D

nmi wrote:

Quote:
Usually only template classes or very small functions of a class are implemented in the header...

You suggest to implement classes inside .c files or to split the definition of a class between a .c file and a .h file?
Like this:
[link]http://www.eventhelix.com/RealtimeMantra/HeaderFileIncludePatterns.htm[/link]

nmi wrote:

Quote:
enum { MAX_TASK_COUNT = 21 };

is this better practice than #define?

stylin wrote:

Quote:
You're coding in C plus plus now, time to drop those unsafe, pain in the butt C arrays. Also, you're limiting your task count - why?

Quote:
So, ckernel::Run executes until there are no more tasks. (What else would you want to do inbetween your task loop? Remember, your tasks are the objects that make up your system, the kernel just sees that they get executed.)

I intend that every task will be a component of an game engine: render, audio, ai or any other "independent" component. So these tasks cannot die and there shouldn't be other dinamic tasks to be added to the task list after the compile time and they should respect a certain order of execution. That's what I try to do, I don't know if this is the best aproach to a task scheduler but probably I will find out soon. This is the reason why I didn't tried to use or containers, which I assume that they can provide a more generic implementation. I also want to implement a scheduler based on time bases, therefore ctask class can contain additional data needed for scheduler.
I will look into the smart pointer matter, but I'm really undecided if I should go for a generic implementation like you suggest or remain with a C-like implementation.

Enigma

Sep 8, 2005 at 8:15 AM

Quote:
Original post by toro
You suggest to implement classes inside .c files or to split the definition of a class between a .c file and a .h file?

Put the class definition:

class X{	public:		X();		int memberFunction():	private:		int variable_;};

in a header, with inclusion guards (as shown in the link you posted) and the class member definitions:

X::X(){	// code}

in a .cpp or .cc file (not a .c, since many C++ compilers are also C compilers and unless told otherwise use the file extension to determine if the file is C++ or C code).

Quote:
nmi wrote:
Quote:
enum { MAX_TASK_COUNT = 21 };

is this better practice than #define?

That or even just unsigned int const MAX_TASK_COUNT = 21 (probably in the anonymous namespace to keep it local to the translation unit) are much better than #define because they are part of the language. #define is part of the preprocessor and is simply text replaced. It therefore ignores scoping, type and context and can result in some nasty errors, i.e.:

// lots of lines of code// in some header file that gets included#define COUNT 7;// lots of lines of codevoid somefunction(){	int const COUNT = numberOfWidgets;}

That's what you see. After the preprocessor has done its work what the compiler sees is:

// lots of lines of codevoid somefunction(){	int const 7 = numberOfWidgets;}

which is obviously nonsensical.

Quote:
I intend that every task will be a component of an game engine: render, audio, ai or any other "independent" component. So these tasks cannot die and there shouldn't be other dinamic tasks to be added to the task list after the compile time and they should respect a certain order of execution. That's what I try to do, I don't know if this is the best aproach to a task scheduler but probably I will find out soon. This is the reason why I didn't tried to use or containers, which I assume that they can provide a more generic implementation. I also want to implement a scheduler based on time bases, therefore ctask class can contain additional data needed for scheduler.
I will look into the smart pointer matter, but I'm really undecided if I should go for a generic implementation like you suggest or remain with a C-like implementation.

For your pctask array a raw stack based array may be appropriate if you know exactly how many tasks there will be. If not, use a SC++L container. As for pointers, it depends. Do you enjoy spending hours in front of the debugger trying to find out where your pointers are being double deleted or leaked or trying to find the cause of an intermittent crash that turns out to be caused by using a pointer after it was deleted? If so then stick with a C-like implementation. If not then grab the boost smart pointer library, use smart pointers and become much more productive.

Enigma

toro

Sep 14, 2005 at 3:58 PM

[one week later]

stylin, I think I will use the vector container after all, a generic solution is better for this case...

enigma, I know that I cannot use something like this:

vector <auto_ptr<classTask> > vTask;

and from what I read, it seems that combining vector with auto_ptr is a bad idea, because auto_ptr is noncopyable, so instead of auto_ptr I will try to use shared_ptr from boost, do u have any ideea if this will work?

toro

Sep 17, 2005 at 12:53 PM

after some learning process about smart pointer and STL, i came up with this solution:

gt_kernel.cpp

#include <memory>#include "gt_kernel.h"using namespace std;classKernel::classKernel(){    smart_ptr_simple<classTask> pTask1(new classGraphic);    smart_ptr_simple<classTask> pTask2(new classGraphic);        AddTask(pTask1);    AddTask(pTask2);}classKernel::~classKernel(){	}void classKernel::Run(){    uint8 indx;        for (indx = 0; indx < vec_task.size(); indx++)    {        vec_task[indx]->Run();    } }void classKernel::AddTask( smart_ptr_simple<classTask> ptr_task){    vec_task.push_back(ptr_task);}

gt_kernel.h

#ifndef GT_KERNEL_H_GUARD_#define GT_KERNEL_H_GUARD_#include <vector>#include <memory>#include "gt_types.h"#include "gt_task.h"#include "gt_graphic.h"#include "gt_smart_ptr_simple.h"using namespace std;class classKernel{    public:    uint16  task_count;    uint16  task_count_int;    uint16  task_count_nint;    uint64  task_time;        private:    std::vector<smart_ptr_simple<classTask> > vec_task;    	public:	classKernel();	~classKernel();	void Run(void);	void AddTask(smart_ptr_simple<classTask> ptr_task);};#endif //GT_KERNEL_H_GUARD_

where the smart_ptr_simple is defined like this:
smart_ptr_simple.h

#ifndef GT_SMART_PTR_SIMPLE_H_GUARD_#define GT_SMART_PTR_SIMPLE_H_GUARD_template <class T>class smart_ptr_simple{	public:	explicit smart_ptr_simple(T* ptr):ptr_(ptr) {};	smart_ptr_simple& operator =(const smart_ptr_simple& other) {return *this;}	~smart_ptr_simple() {};		T* operator->() const {return ptr_;}	T& operator*() const {return *ptr_;}		private:	T*      ptr_;};#endif // GT_SMART_PTR_H_SIMPLE_GUARD_

so, please tell me ur opinion about this implementation...it is better or it only looks better?

as a remark: i know my smart pointer is not very bright, but unfortunate for me i was unable to make boost work [i don't know why] and auto_ptr is not working for containers, so i know i cannot do a lot of pointer operations with this...

Enigma

Sep 17, 2005 at 7:10 PM

How were you trying to install boost? You should just have to download the boost source, download Boost.Jam and run bjam "-sTOOLS=your-toolset" install.

As to your source, it looks better, but there are still some things I would change:

gt_kernel.cpp
classKernel::Run
Does it really help to abbreviate a five letter word to four letters? Also I would prefer to use std::for_each instead of an explicit loop (std::for_each(vec_task.begin(), vec_task.end(), std::mem_fun(&classTask::Run));).

gt_kernel.h
using namespace std;
Never put a namespace using declaration in a header. Imagine you had the following two header files:
```
// my3dvectorclass.htemplate < typename TYPE >class vector{	// 3d vector implementation}// collisionDetection.h#include "my3dvectorclass.h"namespace coldet{	// various collision detection functions	vector< float > collide(object collider, object collidee);}
```
Normally this compiles fine, but as soon as you #include "gt_kernel.h" in the same translation unit as and before collisionDetection.h you bring an extra symbol into the current namespace and introduce an ambiguity in the return type of the collide function (std::vector< float > or the unnamespaced vector< float >).

gt_kernel.h
task_count
If this is supposed to be the number of tasks in the vector of tasks then it definitely shouldn't be public and actually shouldn't exist as a separate variable at all. Just use the vector.size():
```
classKernel kernel;// kernel.task_count should now equal 2// change the task_countkernel.task_count = 7;// kernel still only has two tasks but now has a task_count of seven
```

gt_kernel.h
Variable Names
You seem to be using a form of Hungarian Notation. Frankly who cares that classKernel is implemented as a class? What does it matter that vec_tasks is a vector? It could just as easily be a deque, list, egg_box or multidimensional_doobrey and may well change to become one as your program evolves and your requirements become more refined. All you need to know is that it stores a number of tasks.

smart_ptr_simple.h
I'm afraid your smart pointer implementation isn't just "not very bright", it's positively brain-dead. It offers you nothing over and above a normal raw pointer, and actually offers less (your assignment operator doesn't actually assign)! My first recommendation would be to get boost working (even if you can't get the lib files built and installed properly the smart pointer library will still work, it's header implementation only). Failing that find another smart pointer implementation on the web. Failing that try this minimal (or less than minimal) implementation (lack of bugs not guaranteed):

template < typename TYPE >class shared_ptr{	public:		shared_ptr()			:			pointer_(0),			reference_count(0)		{		}		shared_ptr(TYPE * const pointer)			:			pointer_(pointer),			referenceCount_(new unsigned int(1))		{		}		shared_ptr(shared_ptr< TYPE > const & pointer)			:			pointer_(pointer.pointer_),			referenceCount_(pointer.referenceCount_)		{			++*referenceCount_;		}		~shared_ptr()		{			if (referenceCount_)			{				--*referenceCount_;				if (*referenceCount_ == 0)				{					delete pointer_;					delete referenceCount_;				}			}		}		TYPE * operator->()		{			return pointer_;		}		TYPE const * operator->() const		{			return pointer_;		}		TYPE & operator*()		{			return *pointer_;		}		TYPE const & operator*() const		{			return *pointer_;		}		shared_ptr & operator=(shared_ptr const & pointer)		{			shared_ptr temp(pointer);			temp.swap(*this);			return *this;		}	private:		void swap(shared_ptr & pointer)		{			std::swap(pointer_, pointer.pointer_);			std::swap(referenceCount_, pointer.referenceCount_);		}		TYPE * pointer_;		unsigned int * referenceCount_;};

Enigma

toro

Sep 18, 2005 at 6:00 AM

Thank you Enigma.

Quote:
How were you trying to install boost? You should just have to download the boost source, download Boost.Jam and run bjam "-sTOOLS=your-toolset" install.

I did these steps, but it seems that the *.hpp files are in the wrong directory.

Quote:
classKernel::Run
Does it really help to abbreviate a five letter word to four letters?

I did not understand the above paragraph.

That hybrid hungarian notation is helping me to understand the code better.[I'm really not experienced with C++]

As for the smart pointers I will try to use the boost implementation, if I still don't get it, I will use your implementation. I just want something simple, which I can understand and it seems that ur solution is ok. Thanks.

PS. I figured out the problem with Boost, the problem was that I tried to buid the boost for GNU GCC, but I'm actually using MinGW, so the parameters for bjam are a little bit different.

bjam "-sMINGW_ROOT_DIRECTORY=C:\MinGW" "-sTOOLS=mingw" install
instead of:
bjam "-sGCC_ROOT_DIRECTORY=C:\MinGW" "-sTOOLS=gcc" install

PS2. I think I will not use the hungarian notation anymore..:D

nmi

Sep 21, 2005 at 9:19 AM

Quote:
Original post by toro
... and from what I read, it seems that combining vector with auto_ptr is a bad idea, because auto_ptr is noncopyable ...

Some info on how to use auto_ptr:
http://www.gotw.ca/publications/using_auto_ptr_effectively.htm

After you have applied the changes that Enigma suggested, your code will look good.

If you think about a scheduler, there may be some things to say. A scheduler is responsible to select the next task to execute from a set of tasks.
So a task provides a function that may be executed by the scheduler, while the scheduler will probably need additional information about the task depending on the type of scheduling you are using (EDF, fifo, fixed priority, round robin).

For instance for EDF you will assign a deadline to a task when it was scheduled. Then all tasks are sorted by their deadline and executed in that order. So the scheduler will add some additional data, which the task itself does not need to know. A fifo scheduler in turn will not need to add additional data, but will just remember the task as is.

If you keep your scheduler open to these ideas, you will be able to easily change the implementation of your scheduler, if you want to execute some task more frequent than some other task for example.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.