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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
