Benoit Girard (:BenWa) Assignee
	Description • 8 years ago

STR:
1) Build from 'hg pull -r d27ebada59cbf0228ac2eeb25fbe5863a87844f1 https://reviewboard-hg.mozilla.org/gecko'

	Benoit Girard (:BenWa) Assignee
	Comment 1 • 8 years ago

I don't understand how this code is supposed to work. It looks like we have conflicting definitions of Function.

	Nathan Froyd [:froydnj]
	Comment 2 • 8 years ago

No, that's how things are supposed to work.  We have a declaration of |Function| that takes one argument, and then we specialize it for function types.

What compiler (GCC?) + version is this?

	Benoit Girard (:BenWa) Assignee
	Comment 3 • 8 years ago

I see the same failure on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc4c49cba0df

On my bulldozer machine I have:
gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2

If it saves time I can easily authorized your public key. The machine is accessible via the mozilla VPN.

	Nathan Froyd [:froydnj]
	Comment 4 • 8 years ago

Ah, the MSVC error is clearer:

 14:58:45     INFO -  c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla/Function.h(135) : error C2872: 'Function' : ambiguous symbol
 14:58:45     INFO -          could be 'c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla/Function.h(132) : mozilla::Function'
14:58:45 INFO - or 'c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\dom\base\nsGlobalWindow.h(107) : mozilla::dom::Function' 

We have mozilla::dom::Function:

https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/mozilla/dom/FunctionBinding.h#27

But we have a number of places that (correctly) forward-declare mozilla::dom::Function.  Then when we attempt to forward-declare mozilla::Function, we get complaints about mozilla::dom::Function already existing.  Seems like somebody's namespace resolution algorithm doesn't work correctly?

I'm not really sure what to do about this. :(  I'd be kind of curious if moving the declaration of mozilla::Function to here:

http://mxr.mozilla.org/mozilla-central/source/mfbt/Function.h#41

i.e. prior to the |namespace detail| bit, would do anything different.

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Seems like somebody's namespace resolution algorithm doesn't work
> correctly?

But two compilers? That seems unlikely.

> 
> I'm not really sure what to do about this. :(  I'd be kind of curious if
> moving the declaration of mozilla::Function to here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Function.h#41
> 
> i.e. prior to the |namespace detail| bit, would do anything different.

I don't understand but I tried it and it didn't help.

	Benoit Girard (:BenWa) Assignee
	Comment 6 • 8 years ago

Couldn't this just be someone doing 'using namespace dom' before we include Function.h?

	Nathan Froyd [:froydnj]
	Comment 7 • 8 years ago

(In reply to Benoit Girard (:BenWa) from comment #6)
> Couldn't this just be someone doing 'using namespace dom' before we include
> Function.h?

Ah, that seems like a reasonable hypothesis, especially with unified compilation.

	Nathan Froyd [:froydnj]
	Comment 8 • 8 years ago

But that would still seem to indicate a similar bug in both compilers...

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 8 years ago

It's probably not a compiler bug since it occurs in MSVC and GCC. Mac-clang might just be okay because of the unification/include path being different.

	Benoit Girard (:BenWa) Assignee
	Comment 10 • 8 years ago

I think if we rename mozilla::dom::Function to mozilla::dom::DOMFunction this will go away. IMO this is cleaner since DOMFunction isn't compatible with mozilla::Function/std::Function.

	Benoit Girard (:BenWa) Assignee
	Comment 11 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/38331/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38331/

	Benoit Girard (:BenWa) Assignee
	Comment 12 • 8 years ago

This patch fixes the problem

	Peter Van der Beken [:peterv]
	Comment 13 • 8 years ago

I think we might want to do the translation in codegen, since this is a standard WebIDL type and so we'd have WebIDL files diverging from their specs for a silly reason :-/. I'm going to confer with bz to see if we can do something clever.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 8 years ago

So the problem is that we effectively have two conflicting specs here, right?

On the one hand, there is std::function and we have decided to consistently map std::foo into our code as mozilla::Foo (why not mozilla::foo?).

On the other hand, there is the standard Web IDL Function type, and we map Web IDL Foo into our code as mozilla::dom::Foo.

Is that understanding of the situation correct?   Why are we doing mozilla::Foo instead of mozilla::foo for our equivalents of std::foo?

	Botond Ballo [:botond]
	Comment 15 • 8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Benoit Girard (:BenWa) from comment #6)
> > Couldn't this just be someone doing 'using namespace dom' before we include
> > Function.h?
> 
> Ah, that seems like a reasonable hypothesis, especially with unified
> compilation.

Confirmed. Here's a reduced testcase triggering the gcc error:


// From dom/FunctionBinding.h
namespace mozilla {
  namespace dom {
    class Function {};
  }
}

// From some cpp file, present here thanks to unified compilation
namespace mozilla {
  using namespace dom;
}

// From mfbt/Function.h
namespace mozilla {
  template <typename>
  class Function;

  template <typename R, typename... A>
  class Function<R(A...)> {};
}


Interestingly, the error only occurs if the partial specialization is present; gcc doesn't mind the declaration of the primary template. That, and the fact that clang accepts this, suggests that this is a compiler bug, though I haven't checked with the standard.

	Botond Ballo [:botond]
	Comment 16 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #14)
> Why are we doing
> mozilla::Foo instead of mozilla::foo for our equivalents of std::foo?

I think it's just to have consistent naming conventions throughout our code.

	Botond Ballo [:botond]
	Comment 17 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #14)
> So the problem is that we effectively have two conflicting specs here, right?
> 
> On the one hand, there is std::function and we have decided to consistently
> map std::foo into our code as mozilla::Foo (why not mozilla::foo?).
> 
> On the other hand, there is the standard Web IDL Function type, and we map
> Web IDL Foo into our code as mozilla::dom::Foo.
> 
> Is that understanding of the situation correct?

Yes, except I don't think this is really a conflict. Namespaces were added to C++ precisely to deal with name clashes like this. Since the two Functions are declared in different namespaces (mozilla and mozilla::dom), they should be able to co-exist peacefully.

Rather, I think the problem is our use of "using namespace". In the presence of unified compilation, this isn't a hygienic construct to use.

We have precedent for removing uses of "using namespace" because they caused trouble with name clashes (e.g. bug 1132153). My suggestion for fixing this bug, is to do the same for "using namespace mozilla::dom". Thoughts?

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 8 years ago

That would require either a ton of "using mozilla::dom::Foo" or a ton of explicit namespace-qualification through a huge chunk of our codebase.  Doable, but not exactly desirable....

That said, maybe the fact that some of the relevant code is already inside "namespace mozilla { namespace dom {" will make it better.  It's still a good bit of work to actually try it, of course, before you know how ugly the resulting code is.  :(

	Botond Ballo [:botond]
	Comment 19 • 8 years ago

I don't think it's that bad. These changes are sufficient to get BenWa's patch to compile. WDYT?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 8 years ago

Comment on attachment 8728581 [details] [diff] [review]
Remove some uses of "using namespace dom"

Needing to randomly (in the sense that it depends on unification and whether people include Function.h) sprinkle "dom::" all over DOM code is not great.  :(  People will be confused about why "using namespace mozilla::dom" is ok to do in some dom/ directories but not others, for example....

I mean, feedback+ in the sense that it solves the immediate problem, but I'm not terribly happy with the solution.  Renaming one or the other thing would be a strictly better option in my opinion, in terms of developer ergonomics.

	Botond Ballo [:botond]
	Comment 21 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #20)
> Needing to randomly (in the sense that it depends on unification and whether
> people include Function.h) sprinkle "dom::" all over DOM code is not great. 
> :(  People will be confused about why "using namespace mozilla::dom" is ok
> to do in some dom/ directories but not others, for example....

So my answer to this was going to be "the rule will be that you can't do 'using namespace mozilla::dom' anywhere, because we'll remove it from everywhere", but then I saw that there are ~600 uses of "using namespace mozilla::dom" in the codebase, so that's probably not a reasonable proposition. (Previously, I only searched for "using namespace dom", of which there are 36 uses, which seemed more tractable).

> I mean, feedback+ in the sense that it solves the immediate problem, but I'm
> not terribly happy with the solution.  Renaming one or the other thing would
> be a strictly better option in my opinion, in terms of developer ergonomics.

Given the above, I agree.

The question that remains is which to rename. My understanding from comment 13 is that we can't rename the WebIDL Function type, and so our options are:

  - Hack the WebIDL binding generator to generate a C++ name other than Function
    for the WebIDL Function type.

  - Rename mozilla::Function. We could choose a more verbose name, like
    mozilla::TypeErasedFunction, or break consistency with naming convention 
    and call it mozilla::function.

Do you have a preference?

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 8 years ago

We _can_ rename the Web IDL function type.  It might be a bit confusing to people, but should be doable.  We can avoid the divergence in actual Web IDL files via typedef, at least until we start spitting out C++ typedefs for Web IDL typedefs.

I think my personal preference is to go with mozilla::function, but I'm not set on it.

	Nathan Froyd [:froydnj]
	Comment 23 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #22)
> I think my personal preference is to go with mozilla::function, but I'm not
> set on it.

We would need to do this anyway when we start using std::function...we might as well do it now and get it over with.

	Benoit Girard (:BenWa) Assignee
	Comment 24 • 8 years ago

Ohhh I though it was std::Function which would have had the same problem. If this issue is not case insensitive then that's probably the best approach.

	Jeff Walden [:Waldo]
	Comment 25 • 8 years ago

Moving to std::function is going to require redoing all the |using| statements for Function, isn't it?  Seems like there's work no matter what you do.  And getting rid of |using namespace| has benefits independent of this particular problem point.  I would prefer working to kill off |using namespace| usage.

Frankly, over time I've been warming to what WebKit's WTF does: have all its basic data structures, algorithms, etc. *not* in the main namespace used by WebKit code.  We would end this sort of problem for all time if mfbt stuff were in namespace mfbt.  (We would of course have to *not* do what WTF does, and have |using WTF::Vector;| things in every one of the WTF headers.)

	Botond Ballo [:botond]
	Comment 26 • 8 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25)
> Moving to std::function is going to require redoing all the |using|
> statements for Function, isn't it?

My understanding is the proposal on the table is to rename mozilla::Function to mozilla::function (no namespace change).

> Seems like there's work no matter what
> you do.  And getting rid of |using namespace| has benefits independent of
> this particular problem point.  I would prefer working to kill off |using
> namespace| usage.

Agreed, but it's a lot more work (~600 usages of |using namespace mozilla::dom|) than renaming mozilla::Function (a handful of usages).

	Benoit Girard (:BenWa) Assignee
	Comment 27 • 8 years ago

I agree with Botond. I'm going to write a patch to fix the usage of mozilla::Function. We can spin off the DOM namespace changes in another bug since it has a larger scope and it's required to address this problem.

	Benoit Girard (:BenWa) Assignee
	Comment 28 • 8 years ago

Sorry I meant not required to address this problem.

	Benoit Girard (:BenWa) Assignee
	Comment 29 • 8 years ago

Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38331/diff/1-2/

	Nathan Froyd [:froydnj]
	Comment 30 • 8 years ago

Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

https://reviewboard.mozilla.org/r/38331/#review36211

r=me.  Thanks for doing this!

	Benoit Girard (:BenWa) Assignee
	Comment 31 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba55d7d1bc95&selectedJob=17969215

	Benoit Girard (:BenWa) Assignee
	Comment 32 • 8 years ago

Build failure is coming from a skia build trick. We're colliding with std::function on platforms that have it be because it's import it in the std namespace. I'm using c++ alias declaration to continue to use the same work around.

diff --git a/gfx/skia/skia/include/private/SkTLogic.h b/gfx/skia/skia/include/private/SkTLogic.h

-    using mozilla::Function;
+    // If we have 'using mozilla::function', we're going to collide with
+    // 'std::function' on platforms that have it. Therefore we use a macro
+    // work around.
+    template<typename Signature>
+    using Function = mozilla::function<Signature>;
     #define function Function

 
I'll assume I can carry forward review with this.

	Benoit Girard (:BenWa) Assignee
	Comment 33 • 8 years ago

Comment on attachment 8726923 [details]
MozReview Request: Bug 1253678 - Rename mozilla::Function to mozilla::function. r=nfroyd

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38331/diff/2-3/

	Benoit Girard (:BenWa) Assignee
	Comment 34 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f990b24a5bc

	Pulsebot
	Comment 35 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c4bc792bb5

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c3c4bc792bb5