Convert all uses of NS_NEWXPCOM/NS_DELETEXPCOM to new/delete

RESOLVED FIXED in mozilla2.0b2

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jduell, Assigned: Saint Wesonga)

Tracking

unspecified
mozilla2.0b2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

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

Updated

8 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

8 years ago
Created attachment 450306 [details] [diff] [review]
Patch
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #450306 - Flags: review?(benjamin)

Updated

8 years ago
Attachment #450306 - Flags: review?(benjamin) → review+

Comment 2

8 years ago
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.
(Assignee)

Updated

8 years ago
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?

Comment 4

8 years ago
I don't have a strong opinion.
The current patch doesn't apply cleanly anymore.
Keywords: checkin-needed

Updated

8 years ago
Blocks: 575156
(Assignee)

Comment 6

8 years ago
Created attachment 454541 [details] [diff] [review]
Patch
Attachment #450306 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
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

Updated

8 years ago
OS: Linux → All
Hardware: x86 → All

Comment 8

8 years ago
Comment on attachment 454541 [details] [diff] [review]
Patch

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

Comment 9

8 years ago
Something like?: nsAutoPtr<nsWebBrowserListenerState> state(new nsWebBrowserListenerState());
(Assignee)

Comment 10

8 years ago
(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

8 years ago
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.

Comment 13

8 years ago
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.]
(Assignee)

Comment 15

8 years ago
Created attachment 455965 [details] [diff] [review]
Patch v2

Separate declaration and assignment statements for nsAutoPtr creation.
Attachment #454541 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Landed and backed out again. Build failed on every platform.
Keywords: checkin-needed
(Assignee)

Comment 17

8 years ago
Created attachment 455993 [details] [diff] [review]
Patch v2

Separate declaration and assignment statements for nsAutoPtr creation.
Attachment #455965 - Attachment is obsolete: true
(Assignee)

Comment 18

8 years ago
I have successfully built this on Linux. Should be good to go.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/dd73fb7289a4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.