Author Topic: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)  (Read 8296 times)

0 Members and 1 Guest are viewing this topic.

pkohut

  • Guest
AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« on: February 21, 2010, 10:00:17 AM »
This post started out being a RFC, but as I was writing it up I pretty much answered my own question and now it's more or less a tutorial. Still, think of it as a RFC, just without the question as you'll probably be able to infer the general gist of the now non-question.  :roll:

When acquiring a new object pointer from most ARX api's the object is passed by reference in the function call, and the function's return is usually Acad::ErrorStatus. Being what it is, this doesn't lend itself to using smart pointers very cleanly, and you end up with 2 objects, 1 for the pointer and another for the smart pointer.

Listing 1 is how I normally handle this, as you see there are 2 objects dealing with the same iterator. I have to be sure to NULL out the original pointer once it is handed off to the smart pointer, otherwise I risk using it after the smart pointer is destroyed.

So the deal is I want this handled automatically and after about an hour of futzing around with different methods I settled on listing 2 and 2a.  Bottom line is it's simple, and with the name AssignToSmartPointer I'll be sure to use it only with some sort of smart pointer.

If for some reason the smart pointer constructor fails it is going to throw std::bad_alloc, so as long as the original pointer is good and std::bad_alloc isn't thrown, then the smart pointer can safely be dereferenced.

In the isolated confines of the examples below, this is pretty simple stuff. However, when working inside of an if statement 3 or 4 block levels deep, every little bit of help helps.

Listing 1
Code: [Select]
   AcDbLayerTableIterator * pIterator;
   es = pLayerTable->newIterator(pIterator);
   if(es !=  Acad::eOk)
      return es;

[color=navy]   boost::scoped_ptr<AcDbLayerTableIterator> pIter(pIterator);   // take control of the pointer and set the
   pIterator = NULL;                                             // set the original pointer to 0
[/color]

Listing 2
Code: [Select]
   AcDbLayerTableIterator * pIterator;
   es = pLayerTable->newIterator(pIterator);
   if(es !=  Acad::eOk)
      return es;
[color=navy]
   boost::scoped_ptr<AcDbLayerTableIterator> pIter(AssignToSmartPointer(pIterator));
[/color]

Listing 2a (template definition for AssignToSmartPointer)
Code: [Select]
template<typename T>
inline T * AssignToSmartPointer(T *& pOriginalPointer)
{
   T * pPtr = pOriginalPointer;
   pOriginalPointer = NULL;
   return pPtr;
}

owenwengerd

  • Bull Frog
  • Posts: 441
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #1 on: February 21, 2010, 10:32:21 AM »
I'm not familiar with boost, but any smart pointer that takes ownership of a pointer should have a constructor that accepts a reference to a non-const pointer, then sets the original pointer to NULL after ownership is transferred successfully.

LE3

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #2 on: February 21, 2010, 10:40:42 AM »
I always have followed the rule of opening for read the table, then after the newIterator it is created, to close the table, do the scan, and at the end delete the iterator, I use smart pointers a lot, but never for these cases.

edit: spelling
« Last Edit: February 21, 2010, 12:10:56 PM by LE3 »

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #3 on: February 21, 2010, 03:17:04 PM »
I'm not familiar with boost, but any smart pointer that takes ownership of a pointer should have a constructor that accepts a reference to a non-const pointer, then sets the original pointer to NULL after ownership is transferred successfully.

Good point and I'll investigate further when I get back. Alas, from what I know the shared_ptr template and family do not do this.

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #4 on: February 21, 2010, 03:46:58 PM »
I always have followed the rule of opening for read the table, then after the newIterator it is created, to close the table, do the scan, and at the end delete the iterator

Absolutely!

What prompted this is that the function that creates the iterator calls another function that has the potential to throw. So all the aquired pointer resources are in their own smart pointers. If the throwing function does throw I want the resouces handled, ie. deleted or closed in the case for opened entites (handled with AcDbObjectPointerBase derived classes).  This eliminates the need for try...catch blocks.


It's Alive!

  • BricsCAD
  • Needs a day job
  • Posts: 7033
  • AKA Daniel
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #5 on: February 21, 2010, 05:33:56 PM »
I always have followed the rule of opening for read the table, then after the newIterator it is created, to close the table, do the scan, and at the end delete the iterator

Absolutely!

...



That's not right though, well at least according to the docs.

Quote
When you create a new iterator, you are also responsible for deleting it. A symbol table should not be closed until all of the iterators it has constructed have been deleted

It's Alive!

  • BricsCAD
  • Needs a day job
  • Posts: 7033
  • AKA Daniel
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #6 on: February 21, 2010, 05:42:10 PM »

how about this? (it works)  :laugh:
Code: [Select]
boost::scoped_ptr<AcDbBlockTableRecordIterator> pIter;
if(pCurBtr->newIterator((AcDbBlockTableRecordIterator*&)pIter) != Acad::eOk)
  ...
« Last Edit: February 21, 2010, 05:50:43 PM by Daniel »

owenwengerd

  • Bull Frog
  • Posts: 441
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #7 on: February 21, 2010, 07:02:25 PM »
Daniel, I thought about mentioning that one could just as well add an operator T*&() to the smart pointer class so that the smart pointer could be passed as the argument to newIterator, but this exposes a potential weakness. If the newIterator() function does not return Acad::eOk, then the pointer is invalid and should not be deleted. That means you would have to add code to explicitly release ownership of the pointer without deleting it when the return value is not Acad::eOk (or throw caution to the wind and assume it will always be NULL when the function returns an error).

It's Alive!

  • BricsCAD
  • Needs a day job
  • Posts: 7033
  • AKA Daniel
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #8 on: February 21, 2010, 07:53:09 PM »
... (or throw caution to the wind and assume it will always be NULL when the function returns an error).

Ah, good point! I was assuming(I'm probably wrong) that the pointer would always be NULL, otherwise how would the newIterator() function know what status to return? I.e. either Autodesk has overridden new to return NULL on failure or a std::bad_alloc  is thrown .?.?.

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #9 on: February 21, 2010, 08:50:17 PM »
That's not right though, well at least according to the docs.

Thanks for pointing that out as I've got a couple programs to go fix, and probably read that a long time ago and forgot (didn't pay attention).  Prior to 2005 or so this would not have been an issue cause I practiced single entry single exit programming, but at some point adjusted to having multiple exit points and closing entities as Louis described so there wouldn't be 6+ levels of nested if blocks.

owenwengerd

  • Bull Frog
  • Posts: 441
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #10 on: February 21, 2010, 09:09:40 PM »
The docs are wrong; it is not necessary to hold the collection open while iterating over it. There may be cases where it's a good idea (i.e. when you need to prevent changes until iteration is complete), and I suspect that's where the statement originates.

LE3

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #11 on: February 21, 2010, 09:10:16 PM »

That's not right though, well at least according to the docs.

Quote
When you create a new iterator, you are also responsible for deleting it. A symbol table should not be closed until all of the iterators it has constructed have been deleted


Thank you Dan.

Are you are reading this from an old arxref doc, no?

This is about AcDbLayerTable on the arxref for 2010
Quote
This function creates an object that can be used to iterate through the contents of the table and sets pIterator to point to it. If atBeginning is true the iterator begins its scan at the beginning of the table; otherwise it begins at the end of the table. If the argument skipDeleted is true, the iterator is initially positioned at the first or last non deleted record. Otherwise it is initially positioned at the first or last record, regardless of whether or not it has been deleted.

Each iterator returned from this function counts as a reader of the symbol table. As such, these iterators may only be constructed when the table is open for read. In addition, the table is not considered closed unless all of the iterators it has constructed have been deleted.
 

Never had any issues, on all my previous use of closing the table :) - and yes found some code where I did the close of the table after the delete of the iterator.

What I understood way back then, is that once you open your table for read, and have your pointer to your iterator, the table can be closed, is not needed anymore.

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #12 on: February 21, 2010, 09:14:59 PM »
Thank you Dan.

Are you are reading this from an old arxref doc, no?

This is about AcDbLayerTable on the arxref for 2010

Look in the ARX Developer Guide "Iterators".

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #13 on: February 21, 2010, 09:29:20 PM »
Modififed code below to clarify intent.
... (or throw caution to the wind and assume it will always be NULL when the function returns an error).

Ah, good point! I was assuming(I'm probably wrong) that the pointer would always be NULL, otherwise how would the newIterator() function know what status to return? I.e. either Autodesk has overridden new to return NULL on failure or a std::bad_alloc  is thrown .?.?.

In something like this
Code: [Select]
AcDbLayerTableIterator * pIter[color=red] = 0;[/color]
es = pTable-newIterator(pIter);

there is no guarantee what pIter will be if es is not Acad::eOk, so it's best not to program against an observed behavior.  When changes are made in the future and/or compiler optimization change the observed behaviors, these bugs can be a nightmare to track down.
« Last Edit: February 21, 2010, 10:28:24 PM by pkohut »

LE3

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #14 on: February 21, 2010, 09:31:12 PM »
Thank you Dan.

Are you are reading this from an old arxref doc, no?

This is about AcDbLayerTable on the arxref for 2010

Look in the ARX Developer Guide "Iterators".

Thank you Paul.

Yes, it is also on old arxref's docs that statement, it even have a link to a code sample.

I learn to do my own - don't trust much on the sample code - provided in there and prefer to do my own trials an errors. :)

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #15 on: February 22, 2010, 08:10:54 AM »
... (or throw caution to the wind and assume it will always be NULL when the function returns an error).

Ah, good point! I was assuming(I'm probably wrong) that the pointer would always be NULL, otherwise how would the newIterator() function know what status to return? I.e. either Autodesk has overridden new to return NULL on failure or a std::bad_alloc  is thrown .?.?.

After a couple more hours of tinkering tonight Iím settling on this.  It works for both shared_ptr and scoped_ptr (listing 1).  Listing 1a shows how to use it with AcDbSymbolTablePointer derived tables.  Listing 2 is the template code that makes this work, and goes in a header file.

Note if GetSmartPointerIterator returns Acad::eOk then the iterator pointer handled by the scoped_ptr or shared_ptr will be valid and ready to use.  Otherwise the iterator pointer will be NULL and the scoped_ptr or shared_ptr object will be 0. So usage can be:
   GetSmartPointer<AcDbBlockTableIterator> (pBlockTable, pTblIterator);
   If(pTblIterator) { Ö }
or
   es = GetSmartPointer<AcDbBlockTableIterator> (pBlockTable, pTblIterator);
   If(es == Acad::eOk) { Ö }


Listing 1, inside some function (pLayerTable is an AcDbSymbolTable derived type)
Code: [Select]
    boost::[color=brown]scoped_ptr[/color]<AcDbLayerTableIterator> pIter1;
    es = GetSmartPointerIterator<AcDbLayerTableIterator>(pLayerTable, pIter1);

    boost::[color=brown]shared_ptr[/color]<AcDbLayerTableIterator> pIter2;
    es = GetSmartPointerIterator<AcDbLayerTableIterator>(pLayerTable, pIter2);

Listing 1a (pLayerTable is an AcDbSymbolTablePointer derived type)
Code: [Select]
    boost::scoped_ptr<AcDbLayerTableIterator> pIter1;
    es = GetSmartPointerIterator<AcDbLayerTableIterator>([color=brown]pLayerTable.object()[/color], pIter1);

Listing 2 (header file)
Code: [Select]
template<typename Iterator, typename Table, typename SP>
inline Acad::ErrorStatus GetSmartPointerIterator(const Table * pTable, SP &spIterator)
{
[color=brown]    Iterator * pIterator;
    Acad::ErrorStatus es = pTable->newIterator(pIterator);
    if(es == Acad::eOk)
        spIterator.reset(pIterator);
    return es;[/color]
}

template<typename Iterator, typename Table>
inline Acad::ErrorStatus GetSmartPointerIterator(const Table * pTable, boost::shared_ptr<Iterator> &spIterator)
{
    return GetSmartPointerIterator<Iterator, Table, boost::shared_ptr<Iterator>>(pTable, spIterator);
}

template<typename Iterator, typename Table>
inline Acad::ErrorStatus GetSmartPointerIterator(const Table * pTable, boost::scoped_ptr<Iterator> &spIterator)
{
    return GetSmartPointerIterator<Iterator, Table, boost::scoped_ptr<Iterator>>(pTable, spIterator);
}


Final thoughts:
  • Wanted a single function signature to handle both scoped_ptr and shared_ptr
  • Again I tried 3 or 4 different implementation methods and they were either too messy, or in the case of scoped_ptr's would not work because scoped_ptr cannot be returned from a function since the copy operator is private .
  • Might take a bit to get use to the implementation details being hidden in the template, though it is no different than AcDbObjectPointerBase->acquire hiding its implementation details.
  • Not trilled about the name GetSmartPointerIterator.
  • Template error messages are a bear to figure out.

It's Alive!

  • BricsCAD
  • Needs a day job
  • Posts: 7033
  • AKA Daniel
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #16 on: February 22, 2010, 08:50:34 AM »
The docs are wrong; it is not necessary to hold the collection open while iterating over it. There may be cases where it's a good idea (i.e. when you need to prevent changes until iteration is complete), and I suspect that's where the statement originates.

found this in the header
Code: [Select]
///// AcDbSymbolTableIterator
//
// This class is responsible for iterating over the records in a symbol
// table.
//
// This is a completely "generic" symbol table iterator in that it can
// be used to iterate over the contents of any AcDbSymbolTable subclass.
// However, symbol table specific iterators are defined below.
//
// One major item of note.  The creation of one of these iterators
// requires the opening (for read) of the table being iterated over.
// This read stays in effect until the iterator is destroyed.
//

It's Alive!

  • BricsCAD
  • Needs a day job
  • Posts: 7033
  • AKA Daniel
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #17 on: February 22, 2010, 08:53:31 AM »
After a couple more hours of tinkering tonight Iím settling on this.

Cool, I will have to give this a try  8-)

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #18 on: February 22, 2010, 09:17:55 AM »
found this in the header
Code: [Select]
///// AcDbSymbolTableIterator
//
// This class is responsible for iterating over the records in a symbol
// table.
//
// This is a completely "generic" symbol table iterator in that it can
// be used to iterate over the contents of any AcDbSymbolTable subclass.
// However, symbol table specific iterators are defined below.
//
[color=brown]// One major item of note.  The creation of one of these iterators
// requires the opening (for read) of the table being iterated over.
// This read stays in effect until the iterator is destroyed.[/color]
//

Nice find.  Quick test reveals this may not always be the case.  :-(


Code: [Select]
    Acad::ErrorStatus es;
    AcDbLayerTable * pLayerTable;
    es = pDb->getLayerTable(pLayerTable, AcDb::kForRead);
    if(es != Acad::eOk)
        return es;

[color=brown]    bool bVal = pLayerTable->isReallyClosing();[/color][color=navy] // bVal == true[/color]
    AcDbLayerTableIterator * pIter;
    es = pLayerTable->newIterator(pIter);
[color=brown]    bVal = pLayerTable->isReallyClosing();[/color][color=navy] // bVal == true[/color]
    pLayerTable->close();
    delete pIter;

owenwengerd

  • Bull Frog
  • Posts: 441
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #19 on: February 22, 2010, 09:25:40 AM »
// One major item of note.  The creation of one of these iterators
// requires the opening (for read) of the table being iterated over.
// This read stays in effect until the iterator is destroyed.

That may have been true in R13, but I suspect it is no longer true.

owenwengerd

  • Bull Frog
  • Posts: 441
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #20 on: February 22, 2010, 09:51:51 AM »
I've never monkeyed around with an iterator smart pointer, but this thread got me thinking about how I would want it to work if I did. I think I'd prefer something simpler and more straightforward:
Code: [Select]
AcDbBlockTableRecord* pBlockTable = OpenBlockTable();
{ //local scope for limiting pIter lifetime
  MyBlockTableIteratorPtr pIter;
  Acad::ErrorStatus es = pBlockTable->newIterator( pIter );
  pBlockTable->close();
  if( es != Acad::eOk )
    return es;
  for( pIter->start(); !pIter->done(); pIter->step() )
  {
    //use pIter
    //here we can cleanly exit the scope without leaking anything
  }
} //pIter is automatically destroyed here

Looking at this code, I can see why I never bothered. The only real benefit is that I can cleanly exit the iterator loop without worrying about cleanup.  However, if this is ever necessary, it's not much more difficult to just set an error status, then break out of the loop, delete the iterator, then check the error status and exit at that point if there was an error.

In the end, I'm just not convinced that there is enough bnefit to a smart iterator pointer in most cases to warrant the effort and hiding of the implementation.

pkohut

  • Guest
Re: AcDbSymbolTableIterator and shared_ptr (scoped_ptr as well)
« Reply #21 on: February 22, 2010, 11:28:10 AM »
Looking at this code, I can see why I never bothered. The only real benefit is that I can cleanly exit the iterator loop without worrying about cleanup.  However, if this is ever necessary, it's not much more difficult to just set an error status, then break out of the loop, delete the iterator, then check the error status and exit at that point if there was an error.

In the end, I'm just not convinced that there is enough bnefit to a smart iterator pointer in most cases to warrant the effort and hiding of the implementation.

I agree.  In the situation I'm dealing with there are a number of resources being gathered, each handled with smart pointers of some sort.  The iterators were the only resource that wasn't (past tense) being handled.  During the gathering phase of the program there is a potential for an exception to be thrown, and I want all the gathered resources to be released in such a case.  Without smart pointers this is delt with in try/catch blocks and declared/aquired pointers need to be kept in sync with the catch block throughout an application's development life time and maintance.  So I'm trying to limit my use of try/catch.
« Last Edit: February 22, 2010, 11:35:16 AM by pkohut »