562387 - Convert all uses of NS_NEWXPCOM/NS_DELETEXPCOM to new/delete

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jason Duell]

Deliciously decruftable.  These macros do absolutely nothing but call new/delete.

Comment 1

[Saint Wesonga]

Attachment: Patch

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Justin Wood (:Callek)]

-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?

Comment 4

[Benjamin Smedberg]

I don't have a strong opinion.

Comment 5

[Dão Gottwald [:dao]]

The current patch doesn't apply cleanly anymore.

Comment 6

[Saint Wesonga]

Attachment: Patch

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[[e-mail]]

Comment on attachment 454541 [details] [diff] [review]
Patch

>+        nsAutoPtr<nsWebBrowserListenerState> state = new nsWebBrowserListenerState();
This needs constructor syntax (nsAutoPtr has no copy constructor).

Comment 9

[Philip Chee]

Something like?: nsAutoPtr<nsWebBrowserListenerState> state(new nsWebBrowserListenerState());

Comment 10

[Saint Wesonga]

(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.

Comment 11

[Benjamin Smedberg]

No, it means the MSVC accepts code which is technically illegal (calling the constructor even though no copy-constructor operator= is available).

Comment 12

[Justin Wood (:Callek)]

(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.

Comment 13

[Benjamin Smedberg]

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.

Comment 14

[[e-mail]]

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.]

Comment 15

[Saint Wesonga]

Attachment: Patch v2

Separate declaration and assignment statements for nsAutoPtr creation.

Comment 16

[Dão Gottwald [:dao]]

Landed and backed out again. Build failed on every platform.

Comment 17

[Saint Wesonga]

Attachment: Patch v2

Separate declaration and assignment statements for nsAutoPtr creation.

Comment 18

[Saint Wesonga]

I have successfully built this on Linux. Should be good to go.

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/dd73fb7289a4