Closed
Bug 428465
Opened 17 years ago
Closed 15 years ago
nsTArray should refuse to compile with T=nsAutoString (or work correctly)
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: jruderman, Assigned: Waldo)
References
Details
(Whiteboard: [sg:want P5])
Attachments
(1 file)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
nsTArray<nsAutoString> is unsafe (see bug 427941 comment 0). It should either be made safe (by changing nsTArray, nsAutoString, or both) or refuse to compile.
Comment 1•17 years ago
|
||
Note that nsAutoString is not the only class that can't be moved with memmove. nsAutoTArray is another example, and there are really many more -- including any class that puts itself into a linked list. Normally one calls copy-constructors, and then destructors, when lacking specific knowledge that they don't need to be called.
Updated•17 years ago
|
OS: Mac OS X → All
Hardware: PC → All
Reporter | ||
Updated•17 years ago
|
Summary: nsTArray should refuse to compile with T=nsAutoString → nsTArray should refuse to compile with T=nsAutoString (or work correctly)
Comment 3•17 years ago
|
||
(In reply to comment #1) > Normally one calls copy-constructors, and then destructors, when lacking > specific knowledge that they don't need to be called. So, nsTArrayElementTraits should define an array resize operator, which is then specialised to realloc for known safe types?
Comment 4•17 years ago
|
||
Well, for moz2 we can use static checking to enforce that either 1) the element type is POD 2) nsTArrayElementTraits is specialized Is there anything about this bug that we absolutely need to fix for 1.9?
Updated•17 years ago
|
Flags: wanted1.9.0.x?
Comment 7•16 years ago
|
||
Let's get this in 1.9.1 or later.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Flags: blocking1.9.1?
Updated•16 years ago
|
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Reporter | ||
Updated•15 years ago
|
Blocks: static_analyses
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:want P5]
Assignee | ||
Comment 8•15 years ago
|
||
Bug 503952 reminded me of this bug, and it seemed a little template specialization could win the day here. We're already using it in JS, so it's not like we're going to break anyone who isn't already broken. After some effort and dead-end work trying to do this in nsStringAPI.h, voila! This patch works for me, as evidenced by the following with it applied, then with a little bit of change atop it: host-6-230:~/moz/2 jwalden$ hg di -U 3 diff --git a/layout/build/nsLayoutModule.cpp b/layout/build/nsLayoutModule.cpp --- a/layout/build/nsLayoutModule.cpp +++ b/layout/build/nsLayoutModule.cpp @@ -155,6 +155,8 @@ static NS_METHOD nsEditorControllerConstructor(nsISupports *aOuter, REFNSIID aIID, void **aResult) { + nsTArray<nsAutoString> a; + nsresult rv; nsCOMPtr<nsIController> controller = do_CreateInstance("@mozilla.org/embedcomp/base-command-controller;1", &rv); if (NS_FAILED(rv)) return rv; host-6-230:~/moz/2 jwalden$ make -s -C obj-i386-apple-darwin8.11.1/layout/build/ nsLayoutModule.cpp /Users/jwalden/moz/2/content/base/src/nsXHTMLContentSerializer.h:114: warning: βvirtual void nsXHTMLContentSerializer::SerializeAttributes(nsIContent*, nsIDOMElement*, nsAString_internal&, const nsAString_internal&, nsIAtom*, nsAString_internal&, PRUint32, PRBool)β was hidden /Users/jwalden/moz/2/content/base/src/nsHTMLContentSerializer.h:76: warning: by βvirtual void nsHTMLContentSerializer::SerializeAttributes(nsIContent*, nsIDOMElement*, nsAString_internal&, const nsAString_internal&, nsIAtom*, nsAString_internal&)β ../../dist/include/nsTArray.h: In member function βvoid nsTArray<E>::DestructRange(PRUint32, PRUint32) [with E = nsAutoString]β: ../../dist/include/nsTArray.h:663: instantiated from βvoid nsTArray<E>::RemoveElementsAt(PRUint32, PRUint32) [with E = nsAutoString]β ../../dist/include/nsTArray.h:674: instantiated from βvoid nsTArray<E>::Clear() [with E = nsAutoString]β ../../dist/include/nsTArray.h:267: instantiated from βnsTArray<E>::~nsTArray() [with E = nsAutoString]β /Users/jwalden/moz/2/layout/build/nsLayoutModule.cpp:158: instantiated from here ../../dist/include/nsTArray.h:862: error: no matching function for call to βnsTArrayElementTraits<nsAutoString>::Destruct(nsAutoString*&)β ../../dist/include/nsTString.h:568: note: candidates are: static nsTArrayElementTraits<nsAutoString>::Dont_Instantiate_nsTArray_of<nsAutoString>* nsTArrayElementTraits<nsAutoString>::Destruct(nsTArrayElementTraits<nsAutoString>::Dont_Instantiate_nsTArray_of<nsAutoString>*) make[1]: *** [nsLayoutModule.o] Error 1 make: *** [default] Error 2 Add a smidgen of cleverness and you even get an informative error message, with gcc at least (hopefully other compilers are also supported, if not similar hacks likely exist). Win all around! I don't want to waste the time to recompile right now, but I could tweak the patch and add: template<class A> class Instead_Use_nsTArray_of { int i; }; and use Instead_Use_nsTArray_of<nsTString_CharT> in arguments, to also identify the alternative. If that sounds cool, I can easily make the change, just don't want to spend forever and a day recompiling it if it's not wanted.
Comment 9•15 years ago
|
||
Comment on attachment 392420 [details] [diff] [review] Patch You can probably get away with incomplete types e.g. >+ template<class A> >+ struct Dont_Instantiate_nsTArray_of; >+ static Dont_Instantiate_nsTArray_of<nsTAutoString_CharT> * >+ Construct(Dont_Instantiate_nsTArray_of<nsTAutoString_CharT> e); etc.
Updated•15 years ago
|
Attachment #392420 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9d3f9621b2f2 http://hg.mozilla.org/mozilla-central/rev/51f02ed639c9 Fixed in m-c.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment 11•11 years ago
|
||
Bah, after all that this prevents you from using nsTArray<ns/*C*/String> outside of libxul, the problem being that nsStringAPI.h typedefs nsAuto/*C*/String to ns/*C*/String.
Comment 12•11 years ago
|
||
Ignore me, I think someone changed an nsStringGlue.h to an nsString.h include.
You need to log in
before you can comment on or make changes to this bug.
Description
•