class declared in one namespace and defined in another

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

Trunk
mozilla8
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 546598 [details] [diff] [review]
define Proxy in the namespace it is declared

The class Proxy is declared in mozilla::dom::workers::xhr, but is defined outside of any namespace in XMLHttpRequestPrivate.cpp.

Clang complains about this. It is possible that this is clang bug (http://llvm.org/pr10393), but it IMHO the code is easier to understand anyway if the class is explicitly defined in the same namespace it is declared. The attached does that.
Attachment #546598 - Flags: review?(bent.mozilla)
Comment on attachment 546598 [details] [diff] [review]
define Proxy in the namespace it is declared

Review of attachment 546598 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes:

::: dom/workers/XMLHttpRequestPrivate.cpp
@@ +61,5 @@
>  #include "XMLHttpRequest.h"
>  
> +namespace mozilla {
> +namespace dom {
> +namespace workers {

Please use BEGIN_WORKERS_NAMESPACE and END_WORKERS_NAMESPACE macros for this.

@@ +189,5 @@
>  
> +}
> +}
> +}
> +}

Please make this:

  } // namespace xhr
Attachment #546598 - Flags: review?(bent.mozilla) → review+
Created attachment 546655 [details] [diff] [review]
updated patch
Attachment #546598 - Attachment is obsolete: true
Attachment #546655 - Flags: checkin?
Keywords: checkin-needed
This was backed out. bent, it would probably be best if you merged this into the right patch.
Assignee: nobody → respindola
Blocks: 649537
Status: NEW → ASSIGNED
Keywords: checkin-needed
Version: unspecified → Trunk
Ben, it would be great if we can get this second patch relanded ASAP.  Right now, Firefox trunk fails to build with clang.  :(
The original patch applies cleanly. Can it be checked in?
Attachment #546655 - Flags: review?(bent.mozilla)
Attachment #546655 - Flags: checkin?
Attachment #546655 - Flags: review?(bent.mozilla) → review?(khuey)
Comment on attachment 546655 [details] [diff] [review]
updated patch

If the reviewer gave you r+ the first time they trust you to make the requested changes on your own.
Attachment #546655 - Flags: review?(khuey) → review+
Sorry, I wasn't CC'd, and I forgot :(
Keywords: checkin-needed
Pushed to inbound.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5ec41b0085be
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.