Closed Bug 562387 Opened 10 years ago Closed 9 years ago

Convert all uses of NS_NEWXPCOM/NS_DELETEXPCOM to new/delete

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: jduell.mcbugs, Assigned: wesongathedeveloper)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

Deliciously decruftable.  These macros do absolutely nothing but call new/delete.
Whiteboard: [good first bug]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #450306 - Flags: review?(benjamin)
Attachment #450306 - Flags: review?(benjamin) → review+
Comment on attachment 450306 [details] [diff] [review]
Patch

Thanks! Please announce this change to mozilla.dev.platform, in case others who build on our codebase (mailnews in particular) need to make a similar change.
Keywords: checkin-needed
-adding a few comm-central folks to the cc list.

I also wonder if we can hold off on the define removal for a bit (as in, land this in two parts). Bsmedberg do you think thats doable, or do you want this to land all at once so there is no chance in forgetting about it?
I don't have a strong opinion.
The current patch doesn't apply cleanly anymore.
Keywords: checkin-needed
Blocks: 575156
Attached patch Patch (obsolete) — Splinter Review
Attachment #450306 - Attachment is obsolete: true
Keywords: checkin-needed
landed and backed out because of this:

/builds/moz2_slave/mozilla-central-macosx/build/embedding/browser/webBrowser/nsWebBrowser.cpp: In member function 'virtual nsresult nsWebBrowser::AddWebBrowserListener(nsIWeakReference*, const nsIID&)':
/builds/moz2_slave/mozilla-central-macosx/build/embedding/browser/webBrowser/nsWebBrowser.cpp:239: error: conversion from 'nsWebBrowserListenerState*' to non-scalar type 'nsAutoPtr<nsWebBrowserListenerState>' requested

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1277824704.1277826283.32177.gz
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
Comment on attachment 454541 [details] [diff] [review]
Patch

>+        nsAutoPtr<nsWebBrowserListenerState> state = new nsWebBrowserListenerState();
This needs constructor syntax (nsAutoPtr has no copy constructor).
Something like?: nsAutoPtr<nsWebBrowserListenerState> state(new nsWebBrowserListenerState());
(In reply to comment #8)
> (From update of attachment 454541 [details] [diff] [review])
> >+        nsAutoPtr<nsWebBrowserListenerState> state = new nsWebBrowserListenerState();
> This needs constructor syntax (nsAutoPtr has no copy constructor).

This patch compiled without any errors in Visual C++ Express 2010. Does this imply that MSVC generates incorrect code for this line? I'm not sure why that would work in one compiler and not another.
No, it means the MSVC accepts code which is technically illegal (calling the constructor even though no copy-constructor operator= is available).
(In reply to comment #8)
> (From update of attachment 454541 [details] [diff] [review])
> >+        nsAutoPtr<nsWebBrowserListenerState> state = new nsWebBrowserListenerState();
> This needs constructor syntax (nsAutoPtr has no copy constructor).

Odd, isn't this [basically] exactly what the macro expands out to anyway?

Would
nsAutoPtr<nsWebBrowserListenerState> state;
state = new nsWebBrowserListenerState();

Really cause this to succeed rather than fail?

I'm not saying you or ben are wrong clearly, but I just am seeing a different convention in this problem than I am used to having seen.
Yes, two separate lines cause it to succeed. This is an unfortunate case of the c++ spec being dumb, IIRC, but I don't have the exact reference available.
Foo foo = bar; means the same as Foo foo(static_cast<const Foo&>(bar));
The static cast can be resolved by a cast operator on bar or an implicit constructor of Foo from bar. Although the standard requires the implicit copy constructor to exist, the compiler is allowed to optimise it away by means of return value optimisation or copy elision, even for copy constructors with side effects, so that the result is the same as the direct constructor Foo foo(bar);

[Note that constructor syntax can compile in cases where the assignment syntax fails, such as where an intermediate object is necessary.]
Attached patch Patch v2 (obsolete) — Splinter Review
Separate declaration and assignment statements for nsAutoPtr creation.
Attachment #454541 - Attachment is obsolete: true
Keywords: checkin-needed
Landed and backed out again. Build failed on every platform.
Keywords: checkin-needed
Attached patch Patch v2Splinter Review
Separate declaration and assignment statements for nsAutoPtr creation.
Attachment #455965 - Attachment is obsolete: true
I have successfully built this on Linux. Should be good to go.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/dd73fb7289a4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Blocks: 575197
You need to log in before you can comment on or make changes to this bug.