You are viewing a plain text version of this content. The canonical link for it is here.
Posted to c-dev@xerces.apache.org by David Bertoni <db...@apache.org> on 2008/03/27 18:15:56 UTC
Recent change to MemoryManager.hpp
Hi all,
The recent changes to MemoryManager.hpp break deriving classes. The
following patch allows deriving classes to work without any changes:
Index: MemoryManager.hpp
===================================================================
--- MemoryManager.hpp (revision 641405)
+++ MemoryManager.hpp (working copy)
@@ -62,7 +62,7 @@
*
* @return A pointer to the memory manager
*/
- virtual MemoryManager* getExceptionMemoryManager() = 0;
+ virtual MemoryManager* getExceptionMemoryManager() { return this; }
//
-----------------------------------------------------------------------
I realize this introduces some implementation into the MemoryManager class,
but I think it's worth it for compatibility.
Opinions?
Dave
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Tobias <ub...@gmx.net>.
----- Original Message -----
From: "Boris Kolpackov" <bo...@codesynthesis.com>
To: <c-...@xerces.apache.org>
Sent: Sunday, March 30, 2008 6:20 PM
Subject: Re: Recent change to MemoryManager.hpp
> Plus making the function pure virtual
> is the right thing to do should we be designing the interface
> from scratch.
>
IMHO this is not a question about what is being the right thing,
because this seems to be a preference of the author/committer if not
specified
exactly by the coding style of the underlying project everyone has to
follow.
If one would ask 10 different people one would certainly get 15 different
answers.
In the end I encourage any patch that enables the local memoy management and
I'm happy with the current solution because it got into trunk and satisfies
my needs.
Your recent change to the documentation of getExceptionMemoryManager makes
it even more obvious how to fix the breaking change in custom memory
managers.
tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Boris Kolpackov <bo...@codesynthesis.com>.
Hi Tobias,
Tobias <ub...@gmx.net> writes:
> i agree with dave and still prefer the backward compability from an user's
> point of view,
> because using local memory managers without special patching was definitly
> not possible.
> that's why 100% of all custom memory managers out there are
> exception-neutral and don't care about local memory management at all.
Your mistake is in assuming that everyone realizes that there is a
problem with exception handling and local memory managers. This is
definitely not the case (As I mentioned before I found a potential
bug in my implementation).
If we don't make the function pure virtual we will let all those
users keep using their potentially-buggy implementations without
knowing it. Because there are not many custom memory managers, I
think it is ok to inconvenience a few smart users in order to help
the not so smart ones (including myself) to realize they may have
a problem in their code. Just have some compassion for the less
fortunate ones, guys ;-). Plus making the function pure virtual
is the right thing to do should we be designing the interface
from scratch.
Boris
--
Boris Kolpackov, Code Synthesis Tools
Open source XML data binding for C++: http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Tobias <ub...@gmx.net>.
the original patch I've provided for xercesc-1730 was not pure virtual.
making it pure virtual doesn't fix anything, because the memory management
was not designed for scoping aka local memory management and there are
exactly _zero_
local memory management implementations out there that are using an
unmodified version of xercesc.
i agree with dave and still prefer the backward compability from an user's
point of view,
because using local memory managers without special patching was definitly
not possible.
that's why 100% of all custom memory managers out there are
exception-neutral and
don't care about local memory management at all.
so making it pure virtual only breaks existing code like special-purpose
memory managers for unserialized or
low-fragmentation win32-heaps or other special non-process heap
implementations.
regards
tobias
----- Original Message -----
From: "Boris Kolpackov" <bo...@codesynthesis.com>
To: <c-...@xerces.apache.org>
Sent: Friday, March 28, 2008 4:33 PM
Subject: Re: Recent change to MemoryManager.hpp
> Hi David,
>
> David Bertoni <db...@apache.org> writes:
>
>> I doubt there are many MemoryManager implementations out there period,
>> much
>> less users who are going to be hit by this bug. And they can find it
>> just
>> by testing properly.
>
> We are debating two options:
>
> 1. We should keep the API unchanged in order not to inconvenience a small
> number of users.
>
> 2. We should change the API in order to help a small number of users
> fix a bug.
>
> For me when choosing between a mere inconvenience and a bug fix, the
> answer is quite obvious. I am also pretty sure you would make this
> function pure virtual if you were designing the interface from
> scratch. So let's just do the right thing here.
>
> I've added additional information to the getExceptionMemoryManager
> documentation.
>
> Boris
>
> --
> Boris Kolpackov, Code Synthesis Tools
> Open source XML data binding for C++:
> http://codesynthesis.com/products/xsd
> Mobile/embedded validating XML parsing:
> http://codesynthesis.com/products/xsde
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
> For additional commands, e-mail: c-dev-help@xerces.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Boris Kolpackov <bo...@codesynthesis.com>.
Hi David,
David Bertoni <db...@apache.org> writes:
> I doubt there are many MemoryManager implementations out there period, much
> less users who are going to be hit by this bug. And they can find it just
> by testing properly.
We are debating two options:
1. We should keep the API unchanged in order not to inconvenience a small
number of users.
2. We should change the API in order to help a small number of users
fix a bug.
For me when choosing between a mere inconvenience and a bug fix, the
answer is quite obvious. I am also pretty sure you would make this
function pure virtual if you were designing the interface from
scratch. So let's just do the right thing here.
I've added additional information to the getExceptionMemoryManager
documentation.
Boris
--
Boris Kolpackov, Code Synthesis Tools
Open source XML data binding for C++: http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by David Bertoni <db...@apache.org>.
Boris Kolpackov wrote:
> Hi David,
>
> David Bertoni <db...@apache.org> writes:
>
>> If they were using a local MemoryManager instance in their code, they would
>> likely have already tripped this bug.
>
> I had this bug in my code and was not aware of it until I added that
> function as pure virtual. The reason for this is that exceptions are
> not normmaly thrown so this bug can linger on undetected for a while.
Then you need to step up your testing. I found it in about a day. ;-)
>
>
>> And if they haven't tripped over it, my change doesn't modify the
>> behavior of their code.
>
> You mean you change prevents them from detecting and fixing the bug?
> I don't think it's a very strong motivation ;-).
I'm not a big fan of breaking APIs in general, and I'm even less a fan of
breaking APIs to find what are potentially bugs in client code that can be
avoided and discovered through proper design and testing.
I doubt there are many MemoryManager implementations out there period, much
less users who are going to be hit by this bug. And they can find it just
by testing properly.
Dave
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Boris Kolpackov <bo...@codesynthesis.com>.
Hi David,
David Bertoni <db...@apache.org> writes:
> If they were using a local MemoryManager instance in their code, they would
> likely have already tripped this bug.
I had this bug in my code and was not aware of it until I added that
function as pure virtual. The reason for this is that exceptions are
not normmaly thrown so this bug can linger on undetected for a while.
> And if they haven't tripped over it, my change doesn't modify the
> behavior of their code.
You mean you change prevents them from detecting and fixing the bug?
I don't think it's a very strong motivation ;-).
> I will add some more verbiage and a URL that references the Jira issue.
Sounds good. I agree I should have said that if the memory manager
is used locally then the global memory manager
(XMLPlatformUtils::fgMemoryManager) should probably be used for
exceptions.
Boris
--
Boris Kolpackov, Code Synthesis Tools
Open source XML data binding for C++: http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by David Bertoni <db...@apache.org>.
Boris Kolpackov wrote:
> Hi David,
>
> David Bertoni <db...@apache.org> writes:
>
>> The recent changes to MemoryManager.hpp break deriving classes.
>
> That shouldn't be a problem since 3.0.0 is allowed to break
> interfaces.
Yes, but we shouldn't be gratuitous about it.
>
>
>> - virtual MemoryManager* getExceptionMemoryManager() = 0;
>> + virtual MemoryManager* getExceptionMemoryManager() { return this; }
>>
>> I realize this introduces some implementation into the MemoryManager
>> class, but I think it's worth it for compatibility.
>
> There is a more important drawback in doing this: the pure virtual
> function forces users who implement their own memory managers to
> pay attention and decide what to return from this function. In
> fact, memory managers that are used locally should probably return
> the global memory manager (XMLPlatformUtil::fgMemoryManager), not
> 'this'. Otherwise there is a good chance that the exception thrown
> will outlive the memory manager which manages its guts. So I would
> say we should keep it pure virtual.
If they were using a local MemoryManager instance in their code, they would
likely have already tripped this bug. I'm painfully aware of the problem,
since I ran into in my own code. And if they haven't tripped over it, my
change doesn't modify the behavior of their code.
If we leave this as pure virtual, we need better documentation in the
header file so users understand the issues. Just saying "If the same
memory manager can be used, simply return 'this' from this function" isn't
going to help them make a decision.
I will add some more verbiage and a URL that references the Jira issue.
Dave
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
Re: Recent change to MemoryManager.hpp
Posted by Boris Kolpackov <bo...@codesynthesis.com>.
Hi David,
David Bertoni <db...@apache.org> writes:
> The recent changes to MemoryManager.hpp break deriving classes.
That shouldn't be a problem since 3.0.0 is allowed to break
interfaces.
> - virtual MemoryManager* getExceptionMemoryManager() = 0;
> + virtual MemoryManager* getExceptionMemoryManager() { return this; }
>
> I realize this introduces some implementation into the MemoryManager
> class, but I think it's worth it for compatibility.
There is a more important drawback in doing this: the pure virtual
function forces users who implement their own memory managers to
pay attention and decide what to return from this function. In
fact, memory managers that are used locally should probably return
the global memory manager (XMLPlatformUtil::fgMemoryManager), not
'this'. Otherwise there is a good chance that the exception thrown
will outlive the memory manager which manages its guts. So I would
say we should keep it pure virtual.
Boris
--
Boris Kolpackov, Code Synthesis Tools
Open source XML data binding for C++: http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org