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 Sam Lindley <sa...@redsnapper.net> on 2005/01/13 19:56:18 UTC
XMemory flaws
Is the motivation for the design of XMemory documented anywhere?
There appear to be some basic flaws in its design.
In Item 22 of his new book: Exceptional C++ Style, Herb Sutter (the chair of
the C++ standards commitee) gives some useful guidelines for overloading
new:
1) If you provide any class-specific new, always also provide specific
plain (no-extra-parameters) new.
2) If you provide any class-specific new, then always also provide
class-specific in-place new.
3) If you provide any class-specific new, then consider also providing
class-specific nothrow new in case some users of your class do want to
invoke it; otherwise, it will be hidden by other class-specific overloads of
new.
1) is satisfied by XMemory. XMemory provides a non-standard new:
void* operator new(size_t size, MemoryManager* memMgr);
but also provides an overrided version of plain new:
void* operator new(size_t size);
2) is not satisfied by XMemory. Although a comment in the code claims that
the non-standard new overrides placement new (in-place new in Sutter's
words), this is not the case. Global placement new has the signature:
void* operator new(std::size_t size, void* ptr) throw();
The second argument is a void pointer not a MemoryManager pointer.
3) is not satisfied, but this is unlikely to be a problem in practice as
nothrow new is not very widely used and probably not even supported by some
compilers.
The reason why the failure to satisfy 2) is a problem is alluded to in 3).
Defining a class-specific new hides all of the global versions of new. In
particular this means placement new cannot be used with XMemory objects.
Furthermore, placement new is often used to implement standard library
containers - the library shipped with MSVC++ being a notable example. In
practice, this means that containers such as vector are unusable with
objects derived from XMemory. This was a problem for me when trying to
create a vector of XMLUri objects (I don't see any sensible reason why such
objects shouldn't be stored in a standard vector).
The fix is to add a definition of placement new to the class definition and
simply forward it to the global placement new:
void* operator new(std::size_t size, void* q) throw()
{return ::operator new(size, q);}
The corresponding delete operator should also be defined in case an
exception is thrown during construction:
void operator delete(void* p, void* q) throw()
{::operator delete(p, q);}
This can be hidden for the broken Borland compiler as already done for:
void operator delete(void* p, MemoryManager* memMgr);
Technically (according to the C++ standard) the <new> header should be
included at the top of the file <XMemory.hpp>. In practice this can be
omitted, as can the std:: qualification on size_t and the throw
specifications.
It might be argued that by default XMemory derived objects should never be
constructed by calling placement new. If this is the intention (something I
would disagree with) then a dummy private placement new could be given, as
with the hidden constructors and assignment operator.
Another question is how XMemory objects are supposed to behave in the
presence of operator new[]. Currently, no operator new[] is defined, and
hence the global version will be called. Is this the intention?
IMO forwarding functions for the global placement new and delete should
definitely be added to XMemory.hpp. At any rate, I would suggest that the
design goals of XMemory should be documented somewhere.
Sam
---------------------------------------------------------------------
To unsubscribe, e-mail: xerces-c-dev-unsubscribe@xml.apache.org
For additional commands, e-mail: xerces-c-dev-help@xml.apache.org
Re: XMemory flaws
Posted by Alberto Massari <am...@datadirect.com>.
Hi Sam,
At 18.56 13/01/2005 +0000, Sam Lindley wrote:
>Is the motivation for the design of XMemory documented anywhere?
I digged the archive, and I found the e-mail where Khaled introduced the
new design: http://marc.theaimsgroup.com/?l=xerces-c-dev&m=104991662719824&w=2
There you will find an explanation for the missing array operator new; as
for the placement new, I would say it was forgotten because it was not used
by the Xerces code. I am inclined to add it, unless David or Khaled object
to it.
While we are looking at XMemory, there is also the issue reported by Bob
Buck (http://issues.apache.org/jira/browse/XERCESC-1323) that would be
fixed by removing the protected copy constructor and the unimplemented copy
operator (well, the copy operator is not related to that issue, but
compiling Xerces with Visual C++ with warnings at level 4 prints a lot of
warnings, like the protected copy constructor does).
Recap: I plan to change XMemory to be
--- XMemory.hpp 8 Sep 2004 13:56:25 -0000 1.7
+++ XMemory.hpp 17 Jan 2005 08:51:27 -0000
@@ -73,7 +73,8 @@
#endif
/**
- * This method overrides placement operator new
+ * This method defines a custom operator new, that will use the provided
+ * memory manager to perform the allocation
*
* @param size The requested memory size
* @param memMgr An application's memory manager
@@ -81,6 +82,14 @@
void* operator new(size_t size, MemoryManager* memMgr);
/**
+ * This method overrides placement operator new
+ *
+ * @param size The requested memory size
+ * @param ptr The memory location where the object should be allocated
+ */
+ void* operator new(size_t size, void* ptr);
+
+ /**
* This method overrides operator delete
*
* @param p The pointer to the allocated memory
@@ -90,12 +99,20 @@
//The Borland compiler is complaining about duplicate overloading of
delete
#if !defined(XML_BORLAND)
/**
- * This method provides a matching delete for the placement new
+ * This method provides a matching delete for the custom operator new
*
* @param p The pointer to the allocated memory
* @param memMgr An application's memory manager
*/
void operator delete(void* p, MemoryManager* memMgr);
+
+ /**
+ * This method provides a matching delete for the placement new
+ *
+ * @param p The pointer to the allocated memory
+ * @param ptr The memory location where the object had to be allocated
+ */
+ void operator delete(void* p, void* ptr);
#endif
//@}
@@ -108,15 +125,11 @@
//@{
/**
- * Protected default constructor and copy constructor
+ * Protected default constructor
*/
XMemory()
{
}
-
- XMemory(const XMemory&)
- {
- }
//@}
#if defined(XML_BORLAND)
@@ -125,11 +138,6 @@
}
#endif
-private:
- // -----------------------------------------------------------------------
- // Unimplemented operators
- // -----------------------------------------------------------------------
- XMemory& operator=(const XMemory&);
};
XERCES_CPP_NAMESPACE_END
--- XMemory.cpp 6 Jan 2005 21:39:44 -0000 1.12
+++ XMemory.cpp 17 Jan 2005 08:51:24 -0000
@@ -69,6 +69,11 @@
return (char*)block + headerSize;
}
+void* XMemory::operator new(size_t size, void* ptr)
+{
+ return ptr;
+}
+
void XMemory::operator delete(void* p)
{
if (p != 0)
@@ -105,6 +110,10 @@
MemoryManager* pM = *(MemoryManager**)block;
pM->deallocate(block);
}
+}
+
+void XMemory::operator delete(void* p, void* /*ptr*/)
+{
}
#endif
Feedback?
Alberto
>There appear to be some basic flaws in its design.
>
>In Item 22 of his new book: Exceptional C++ Style, Herb Sutter (the chair
>of the C++ standards commitee) gives some useful guidelines for
>overloading new:
>
> 1) If you provide any class-specific new, always also provide specific
> plain (no-extra-parameters) new.
> 2) If you provide any class-specific new, then always also provide
> class-specific in-place new.
> 3) If you provide any class-specific new, then consider also providing
> class-specific nothrow new in case some users of your class do want to
> invoke it; otherwise, it will be hidden by other class-specific overloads
> of new.
>
>1) is satisfied by XMemory. XMemory provides a non-standard new:
>
> void* operator new(size_t size, MemoryManager* memMgr);
>
>but also provides an overrided version of plain new:
>
> void* operator new(size_t size);
>
>2) is not satisfied by XMemory. Although a comment in the code claims that
>the non-standard new overrides placement new (in-place new in Sutter's
>words), this is not the case. Global placement new has the signature:
>
> void* operator new(std::size_t size, void* ptr) throw();
>
>The second argument is a void pointer not a MemoryManager pointer.
>
>3) is not satisfied, but this is unlikely to be a problem in practice as
>nothrow new is not very widely used and probably not even supported by
>some compilers.
>
>The reason why the failure to satisfy 2) is a problem is alluded to in 3).
>Defining a class-specific new hides all of the global versions of new. In
>particular this means placement new cannot be used with XMemory objects.
>Furthermore, placement new is often used to implement standard library
>containers - the library shipped with MSVC++ being a notable example. In
>practice, this means that containers such as vector are unusable with
>objects derived from XMemory. This was a problem for me when trying to
>create a vector of XMLUri objects (I don't see any sensible reason why
>such objects shouldn't be stored in a standard vector).
>
>The fix is to add a definition of placement new to the class definition
>and simply forward it to the global placement new:
>
> void* operator new(std::size_t size, void* q) throw()
> {return ::operator new(size, q);}
>
>The corresponding delete operator should also be defined in case an
>exception is thrown during construction:
>
> void operator delete(void* p, void* q) throw()
> {::operator delete(p, q);}
>
>This can be hidden for the broken Borland compiler as already done for:
>
> void operator delete(void* p, MemoryManager* memMgr);
>
>Technically (according to the C++ standard) the <new> header should be
>included at the top of the file <XMemory.hpp>. In practice this can be
>omitted, as can the std:: qualification on size_t and the throw specifications.
>
>It might be argued that by default XMemory derived objects should never be
>constructed by calling placement new. If this is the intention (something
>I would disagree with) then a dummy private placement new could be given,
>as with the hidden constructors and assignment operator.
>
>Another question is how XMemory objects are supposed to behave in the
>presence of operator new[]. Currently, no operator new[] is defined, and
>hence the global version will be called. Is this the intention?
>
>IMO forwarding functions for the global placement new and delete should
>definitely be added to XMemory.hpp. At any rate, I would suggest that the
>design goals of XMemory should be documented somewhere.
>
>Sam
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: xerces-c-dev-unsubscribe@xml.apache.org
>For additional commands, e-mail: xerces-c-dev-help@xml.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: xerces-c-dev-unsubscribe@xml.apache.org
For additional commands, e-mail: xerces-c-dev-help@xml.apache.org