Last Comment Bug 672313 - class declared in one namespace and defined in another
: class declared in one namespace and defined in another
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: new-web-workers
  Show dependency treegraph
 
Reported: 2011-07-18 12:21 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-07-27 03:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
define Proxy in the namespace it is declared (886 bytes, patch)
2011-07-18 12:21 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
bent.mozilla: review+
Details | Diff | Splinter Review
updated patch (1.42 KB, patch)
2011-07-18 15:53 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-07-18 12:21:42 PDT
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.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-18 15:34:04 PDT
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
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-07-18 15:53:45 PDT
Created attachment 546655 [details] [diff] [review]
updated patch
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-07-19 02:58:53 PDT
This was backed out. bent, it would probably be best if you merged this into the right patch.
Comment 4 :Ehsan Akhgari 2011-07-26 07:10:40 PDT
Ben, it would be great if we can get this second patch relanded ASAP.  Right now, Firefox trunk fails to build with clang.  :(
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-07-26 09:03:48 PDT
The original patch applies cleanly. Can it be checked in?
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-26 09:17:55 PDT
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.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-26 09:58:34 PDT
Sorry, I wasn't CC'd, and I forgot :(
Comment 8 :Ehsan Akhgari 2011-07-26 14:33:54 PDT
Pushed to inbound.
Comment 9 Marco Bonardo [::mak] 2011-07-27 03:40:55 PDT
http://hg.mozilla.org/mozilla-central/rev/5ec41b0085be

Note You need to log in before you can comment on or make changes to this bug.