Topic: ResbufList class

[It's Alive!]

updated

[LE3]

Hi Dan,

First bird eyes looks good.

Once I get the chance to implement some autolisp functions again from one of my projects, will give a full try out - you should have written this class like seven years ago .

Is this yours? (I am asking because do not see your name on it).

Thanks!

[It's Alive!]

Hi Dan,

First bird eyes looks good.

Once I get the chance to implement some autolisp functions again from one of my projects, will give a full try out - you should have written this class like seven years ago .

Thanks! It's not only for lisp, it should be useful when working with XData, XRecords, Selection Set filters... or where you need to use a resbuf..

Is this yours? (I am asking because do not see your name on it).

of course its not mine, its yours, that's why I wrote it (note the "DRM" on the header) 
...well with the exception of the "detach" method is lifted from a ARX sample.. 

[LE3]

oops did not notice those were your initials...

was thinking on one of the projects I am working that will have the need to export several functions to autolisp.

Hi Dan,

First bird eyes looks good.

Once I get the chance to implement some autolisp functions again from one of my projects, will give a full try out - you should have written this class like seven years ago .

Thanks! It's not only for lisp, it should be useful when working with XData, XRecords, Selection Set filters... or where you need to use a resbuf..

Is this yours? (I am asking because do not see your name on it).

of course its not mine, its yours, that's why I wrote it (note the "DRM" on the header) 
...well with the exception of the "detach" method is lifted from a ARX sample.. 

[pkohut]

up for pier review  

Do you have some test code you can provide, that shows intended usage?

Code: [Select]

class CResbufList //derive from CObject or not??Gut says not.

At first I wasn't to crazy about, as it seemed a bit over baked and heavy for dealing with resbufs, but looking at it closer it has potential.  So these comments are purely after looking at it for about 15 minutes.

1) Eat the dog food you give your dog.  You provide the user of the class an iterator of begin() and end().  Use these in your loops as well, for instance the functions CResbufList::at, clone, and others.  You'll ensure they work as intended as well as provide users of the code ready made examples on how to use them.

2) Maybe add a size function to the class? Sorry just saw the length function, not a very good function name to conform with the std.

3) CResbufList::at and removeAt, kind of useless function taking size_t input parameters as there isn't a find function to know what index to delete.

4) CResbufList::at and removeAt, more useful if they took pointers or iterators at input parameters. (edit: added iterators)

5) Maybe add an insert function.

6) By inference of cloneNode your copy constructor and assignment operators are not exception safe. Make sure you check all API return codes and pointers, you've got a few de-referenced pointers that are not checked first.

Otherwise looks good Daniel.

[pkohut]

Sorry just saw your sample usage at the top of the thread.

Function release leaves mpTail dangling.

Most of the functions returning resbuf pointers should return iterators instead.  I think that would solve a nagging issue with the class, and that's that the class does not encapsulate the resbuf pointer list enough. By returning iterators instead of raw pointers you are enforcing the notion that this is a container class and work to be done on resbuf lists should be done exclusively through the class (as much as possible).  To further enhance the utility of the container class, a CResbuf type class would be a useful addition, probably more useful than the container class itself.

[It's Alive!]

Great feedback! Thank you !!!!!
making changes...

[It's Alive!]

Item 6, how do I make the cctor and assignment operator  "exception safe"? , this is a nullable type.
There shouldn't be any dereferenced pointers 

[pkohut]

You should check the return values of functions that return a newed buffer, acutNewRb acutNewString, etc.  Rule of pointers is never dereference a NULL pointer.  Look at cloneNode.

[It's Alive!]

You should check the return values of functions that return a newed buffer, acutNewRb acutNewString, etc.  Rule of pointers is never dereference a NULL pointer.  Look at cloneNode.

I see it now 

[owenwengerd]

I agree with Paul that you should keep the raw resbufs insulated from the client. I don't like your dependency on MFC. At the very least, I would make that dependency conditional so that the code can work correctly in a non-MFC project. Better would be to remove the dependency altogether.

[It's Alive!]

...I don't like your dependency on MFC. At the very least, I would make that dependency conditional so that the code can work correctly in a non-MFC project. Better would be to remove the dependency altogether.

ACK   Do you mean toss the CString?

[It's Alive!]

Thanks again for all the feedback, I nixed the tostring and removeAt methods,
the functions that return resbufs are now private, the others return iterators.

[gswang]

How to use ResbufList's remove method?
If i want to convert ResbufList_iter object into resbuf* , how to do it?

[It's Alive!]

How to use ResbufList's remove method?
If i want to convert ResbufList_iter object into resbuf* , how to do it?

You should be able to cast a ResbufList_iter to a resbuf

[gswang]

命令: (resbuf_remove 1 2 3 4 5)
(1 2 3 4 5)     ；Not what i want.

   static int ads_resbuf_remove(void)
   {
      ResbufList _list = acedGetArgs();

      ResbufList args = _list;
      ResbufList_iterator iter = args.begin();

      int i = 0;
      for(; iter != args.end(); ++iter)
      {
         if (i == 1)
            _list.remove(const_cast<resbuf*>(&(*iter)));

         i++;
      }
      
      acedRetList(_list);

      return (RSRSLT);
   }

[It's Alive!]

Ah! I see the problem I will fix it tonight

[gswang]

Thank you Daniel, recently busy, right?

[It's Alive!]

Try it now 

Code: [Select]

// (rbtest 1 2 3 4 5 6 7 8 9 0)
  static int ads_rbtest(void)
   {
      ResbufList args = acedGetArgs();

      //added this
      args.removeAt(1);

      ResbufList_iterator iter = args.begin();
      int i = 0;
      for(; iter != args.end(); ++iter)
      {
        if (i == 1)
             args.remove(iter);
         i++;
      }
      acedRetList (args);
      return (RSRSLT);
   }


[It's Alive!]

BTY, I changed a couple of function names  :whistle:

[Kerry]

BTY, I changed a couple of function names  :whistle:

.. and here's the documentation

[It's Alive!]

I know, I know.... but I did comment the code (a little) 

[It's Alive!]

I changed push_back(resbuf*)  to only accept a single node (for performance) and added  append_front(resbuf*) and append_back(resbuf*)  for chains.

[Kerry]

I know, I know.... but I did comment the code (a little) 

It's cool Daniel ... I'm just stirring 

[gswang]

 :?The push_back function could not work correctly!
ResbufList returnBuffer;
for(size_t idx =0; idx<points.length(); idx++)
{
resbuf *pTmp = acutNewRb(RT3DPOINT);
pTmp->resval.rpoint[0] = points[idx].x;
pTmp->resval.rpoint[1] = points[idx].y;
pTmp->resval.rpoint[2] = points[idx].z;
returnBuffer.push_back(pTmp);
}

[It's Alive!]

fixed, try it now

[frtfff]

 

[It's Alive!]

why the long face?

[pkohut]

A year after the comments and I finally get to take at look at the changes you made. Looks good Daniel.

Code: [Select]

inline ResbufList_iterator ResbufList::begin()
{
  return (mpHead == NULL ? NULL : mpHead);
}
Compiler will probably optimize to:
   return mpHead;
but if not then it's a wasted if statement.


The loops structures in ResbufList::at and ResbufList::removeAt can become -

Code: [Select]

ResbufList_iterator ResbufList::at( const size_t idx )
{ 
  for(size_t nItems = 0, resbuf *pRbTemp = mpHead ; pRbTemp != NULL ; pRbTemp = pRbTemp->rbnext, ++nItems)
  {
    if(idx == nItems)
      return pRbTemp;
  }
  return NULL;
}


[frtfff]

Good code,but I don't know how to use.

[pkohut]

Good code,but I don't know how to use.

See this post for ResbufList in use.
http://www.theswamp.org/index.php?topic=28286.0

[It's Alive!]

Thanks Paul!

On your second entry, will that even compile? I always thought those args in the for loop had to be of the same type. 

Dan

[pkohut]

You're right. The declaration of nItems should be outside, however the assignment will still work inside.

Code: [Select]

size_t nItems = 0;
for(resbuf *pRbTemp = mpHead ; pRbTemp != NULL ; pRbTemp = pRbTemp->rbnext, ++nItems)
{
    if(idx == nItems)
        return pRbTemp;
}

... do lots of work

for(nItems =0, resbuf *pRbTemp = mpHead ; pRbTemp != NULL ; pRbTemp = pRbTemp->rbnext, ++nItems)
{
    if(idx == nItems)
        return pRbTemp;
}