Re: [PATCH] Bug #2583 fix: pure virtual method called

On 08/22/2010 01:08 AM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> Bug #2583 fix: pure virtual method called
>>
>> When a cbdata-protected class holds its own cbdata and has virtual
>> toCbdata(), there is a catch22 problem: we need cbdata to know whether
>> the pointer to the class object is valid, and we need to dereference
>> that pointer to get cbdata.
>>
>> Added CbcPointer class to hold both a pointer to a potentially freed
>> class object and the cbdata pointer protecting that object. Keeping
>> the cbdata pointer allows us to test whether the object is still there
>> without dereferencing the object pointer.
>>
>> Use the CbcPointer class to hold safe pointers to AsyncJobs. This
>> prevents "pure virtual method called" failures because we no longer
>> dereference freed job pointers.
>>
>> Removed Initiator parameter from many initiatee constructors. The
>> Adaptation::Initiator::initiateAdaptation method now sets the
>> initiator of the job. This makes the constructor profile simpler and
>> removes the need to propagate Initiator changes through all the
>> [nested] constructors.
>>
>> Renamed AsyncJob::AsyncStart() to AsyncJob::Start(). I had to change
>> the callers code anyway and it was a good opportunity to remove the
>> redundant "Async".
>>
>>
>> Special thanks to Stefan Fritsch for updating and testing an earlier
>> version of this patch.
>>
>>
>> -------------------
>>
>> P.S. The underlying changes have been tested in production but I had
>> to polish them before submitting to trunk. Some advanced C++ concepts
>> might not compile on old/broken compilers. We will try to
>> remove/change them if needed.
>>
>> P.P.S. This change is required for at least one more bug fix and for
>> the the 1xx handling patch.
>>
>
>
> One problem:
>
> src/base/CbcPointer.h:
> * #include "TextException.h" needs to be "base/TextException.h".
> NP: this should be failing on build since -I. is undefined in src/.

Fixed. I do not know why the builds (and make check) did not fail.

> The rest is just more polish
>
> Several of the files with added #include entries:
>
> * For clarity as they grow its useful to keep the #include statements
> alphabetical. The old code remains messy, but no need to mess the nice
> clean new code. :)

I have moved new #include entries where they were out of the
alphabetical order. I was not aware of this documented requirement, sorry.

> * If this is for order-dependent includes that is a violation of the
> testHeaders dependency guarantee in all src/* .h code.
>
> Ditto for *_SOURCES list in src/base/Makefile.am.

Moved.

> src/CommCalls.h:
> * useless whitespace addition.

Fixed, sorry.

> Other than that +1.

Committed to trunk as r10779.

Thank you,

Alex.
Received on Mon Aug 23 2010 - 23:18:25 MDT