Thread: Combine alike functions

Well if you separate the idea of "list" from the "data", then you could have a generic list function to free any list.

Oh, and using recusion here just sucks.

Try a loop, quicker and no danger of smashing your stack with a really long list.

Code:

void FreeOutputList(DFNOUTPUT *tmp)
{
  while ( tmp != NULL ) {
    DFNOUTPUT *next = tmp->next;
    free ( tmp );
    tmp = next;
  }
}

Code:

void FreeNode( void *tmp, int specifier )
{
    DFNCOL *colnode = tmp;
    DFNOUTPUT *outnode = tmp;

    if( specifier == somethingthatmeansDFNCOL && colnode->ptr )
        FreeNode( colnode->ptr, specifier );
    if( specifier == somethingthatmeansDFNOUTPUT &&outnode->ptr )
        FreeNode( outnode->ptr, specifier );
    free( tmp );
}

Or maybe we like something like this...

Code:

void FreeNode( void *tmp, int specifier )
{
    if( tmp )
    {
        DFNCOL *colnode = tmp;
        DFNOUTPUT *outnode = tmp;

        FreeNode( specifier == somethingthatmeansDFNCOL && colnode->ptr
            ? colnode->ptr
            : outnode->ptr
            , specifier );
        free( tmp );
    }
}

That should do it.

[edit] Curses, foiled again! [/edit]


Quzah.

I'd thought it was a good idea to use recursion but seemingly Salem doesn't agree with me.

Most people won't. Recursion should be used sparingly and only when there's a significant improvement over the non-recursive solution. In this case recursion doesn't simplify the solution enough to be worth the cost. The cost is that your list can't be too long or you'll probably get some kind of stack overflow error at runtime. You also end up using more memory and taking more time to create and maintain a separate frame for each recursive call to the function.

You'll also have lots of fun with a circular list. Of course, the loop is going to murder your program with a circular list too...


Quzah.

If the use of recursion is so dangerous, do you have any idea to make this done without recursion.

Explanation:
I want to display a column on the screen, which will be divided into many block filled with different colors. Since the number of blocks varys from time to time, so I cannot just set a fixed number for it because if the number could be smaller than needed, or too large and waste memory.

Code:

void ColInit(DFNCOL *tmp, int m)
{
  tmp->n           = m;
  tmp->rect.left   = xScreen / 3;
  tmp->rect.right  = xScreen / 3 + COLWIDTH;
  tmp->rect.top    = yScreen - COLMARGIN - (int)((heightRect * (m + 1)) + 0.5);
  tmp->rect.bottom = yScreen - COLMARGIN - (int)(heightRect * m + 0.5);
                                  /* plus 0.5 to round the value */
  m++;

  if (m < nRects) {
    tmp->ptr = malloc(sizeof(DFNCOL));
    memset(tmp->ptr, 0, sizeof(DFNCOL));
    ColInit(tmp->ptr, m);
  }
}

Don't cast malloc().

Simple.

Code:

void ColInit ( DFNCOL *tmp, int m )
{
    while(1) {
	tmp->n = m;
	tmp->rect.left      = xScreen / 3;
	tmp->rect.right    = xScreen / 3 + COLWIDTH;
	tmp->rect.top      = yScreen - COLMARGIN -
                                            ( int ) ( ( heightRect *  ( m + 1 ) ) + 0.5 );
                                            // plus 0.5 to round the value
	tmp->rect.bottom   = yScreen - COLMARGIN -  ( int ) ( heightRect * m + 0.5 );
	m++;

	if ( m < nRects )
	{
		tmp->ptr =  /*( DFNCOL * )*/malloc ( sizeof ( DFNCOL ) );
		memset ( tmp->ptr, 0, sizeof ( DFNCOL ) );
		tmp = tmp->ptr;
	}
                else break;
    }
}

Something like that. And try not to use tabs, either.

If the use of recursion is so dangerous

It's not dangerous unless you abuse it to make it so. But recursion isn't always an appropriate solution. Especially with linked lists.

do you have any idea to make this done without recursion.

If the last thing you do is make a recursive call, you can easily replace it with a loop. I took some creative liberty with your code:

Code:

void ColInit(DFNCOL *tmp, int m) {
  do {
    tmp->n             = m;
    tmp->rect.left     = xScreen / 3;
    tmp->rect.right    = xScreen / 3 + COLWIDTH;
    tmp->rect.top      = yScreen - COLMARGIN -
      (int)((heightRect * (m + 1)) + 0.5);
    tmp->rect.bottom   = yScreen - COLMARGIN - 
      (int)(heightRect * m + 0.5);

    if (++m < nRects) {
      tmp->ptr = malloc(sizeof *tmp->ptr);
      // memset() probably isn't a good idea
      memset(tmp->ptr, 0, sizeof *tmp->ptr);
      tmp = tmp->ptr;
    }
  } while (m < nRects);
}

I commented memset() instead of removing it because I don't know what the members of DFNCOL are. I'm 99.9% sure that it's dangerous though. First, there may be padding bits that you shouldn't touch in a structure. Second, all bits zero is only sure to be correct on integral types. So if your member variables aren't all integral, the program may not be doing what you think it's doing. For example, if you use memset on a next pointer in a linked list, the result might not be a null pointer. Those are tricky bugs to find.

How about calloc()?

Use spaces instead. Easier to read when moving code between editors (or onto a forum).


Quzah.

How about calloc()?

Same thing. calloc() is basically malloc() that calls memset(). The problem is that memset() fills the memory with all bits zero. Since a null pointer doesn't necessarily point to address 0x0, and the bit pattern for 0.0 in floating-point might not be all bits zero, you can't expect memset() to work all the time on those types. In fact, you can't expect it to work on anything more complicated than integral types. Fun, huh?

To Narf, sorry, I don't get your point. If memset is not needed, why calloc does the same thing?

Yes. I got it. I replied too quickly prior to reading through the posts.