Closed Bug 1189967 Opened 4 years ago Closed 4 years ago

Char16.h includes <string> even when MOZ_NO_MOZALLOC is defined

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bas.schouten, Assigned: glandium)

References

Details

Attachments

(4 files, 4 obsolete files)

This doesn't work since our <string> wrapper errors out when MOZ_NO_MOZALLOC is defined.
Attachment #8641911 - Flags: review?(mh+mozilla)
Comment on attachment 8641911 [details] [diff] [review]
Don't include string when MOZ_NO_MOZALLOC is defined

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

::: mfbt/Char16.h
@@ +47,5 @@
>  #    define MOZ_USE_CHAR16_WRAPPER
>  #  endif
>  #endif
>  
> +#if defined(MOZ_USE_CHAR16_WRAPPER) && !defined(MOZ_NO_MOZALLOC)

... except the result is also to use char16_t* for char16ptr_t, which is likely not desirable.

You're failing to mention what you're trying to fix here. MOZ_NO_MOZALLOC should almost never be used.
Attachment #8641911 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #1)
> Comment on attachment 8641911 [details] [diff] [review]
> Don't include string when MOZ_NO_MOZALLOC is defined
> 
> Review of attachment 8641911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Char16.h
> @@ +47,5 @@
> >  #    define MOZ_USE_CHAR16_WRAPPER
> >  #  endif
> >  #endif
> >  
> > +#if defined(MOZ_USE_CHAR16_WRAPPER) && !defined(MOZ_NO_MOZALLOC)
> 
> ... except the result is also to use char16_t* for char16ptr_t, which is
> likely not desirable.
> 
> You're failing to mention what you're trying to fix here. MOZ_NO_MOZALLOC
> should almost never be used.

Fair point, so xpcom/typelib/xpt is compiled with MOZ_NO_ALLOC, yet it also includes mozilla-config.h which in turn includes Char16.h. On earlier versions of Visual Studio this would not include <string> and therefor not touch any of our STL wrappers. In newer versions it could, this means mozilla-config.h will now conflict with MOZ_NO_MOZALLOC since out <string> STL wrapper #errors on MOZ_NO_MOZALLOC not being defined.
Flags: needinfo?(mh+mozilla)
Sounds to me it's time to fix bug 1138321.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #3)
> Sounds to me it's time to fix bug 1138321.

Anything I can do to help there? I'd like VS2015 to build :).
So this causes additional issues, since we force-include mozilla-config.h in a bunch of places, this causes us to draw in mozalloc declarations all over the place (for example stuff in testing/tools) where we don't actually link against MOZALLOC or libxul.. Causing linking issues later on.
Basically currently the only way to make this work is defining MOZ_NO_MOZALLOC in all the tests that don't link against mozalloc (since we include mozilla-config.h in all of them), or start linking them against mozalloc..
Flags: needinfo?(mh+mozilla)
Blocks: VC14
So this is one of the solutions to the problem I can come up with, a lot of these cases have stand-alone tests, they get built before mozalloc even exists and so can't easily be linked against it without changing build order. At the same time we probably don't just want to disable STL_WRAPPERS as some of these files are actually also linked into xul.dll. Having said that it's questionable whether there's a point in allowing the STL wrappers as any use of STL headers would cause the links of these stand-alone tests to fail anyway.
Attachment #8641911 - Attachment is obsolete: true
Attachment #8643996 - Flags: review?(mh+mozilla)
The snippet including mozilla/Char16.h was added in bug 895047, which
also made mozilla/Char16.h define __PRUNICHAR__ and typedef PRUnichar
itself. That part was removed in bug 956507, and there is actually no
reason anymore to ensure mozilla/Char16.h is included before prtypes.h.
In fact, doing so presently doesn't change anything for prtypes.h.

This simplifies the problem Bas is trying to fix, and I suspect it's
actually enough to fix it. Bas. Can you check?
Flags: needinfo?(mh+mozilla)
Attachment #8647283 - Flags: review?(Pidgeot18)
Attachment #8647283 - Flags: feedback?(bas)
Comment on attachment 8647283 [details] [diff] [review]
Stop including mozilla/Char16.h from mozilla-config.h

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

Interestingly this patch makes things worse, as it causes a bunch of other places in the build to now include out STL wrappers in more inconvenient places where they cause more, new and exciting build errors :-(. (of the type in bug 1189891)
Attachment #8647283 - Flags: feedback?(bas) → feedback-
Flags: needinfo?(mh+mozilla)
I think it just triggers bug 1120793 earlier than it normally happens.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #10)
> I think it just triggers bug 1120793 earlier than it normally happens.

I had the second patch of that bug in my tree (skip string for mozalloc one), it also built fine with my patch (from this bug) applied which shouldn't really fix anything bug 1120793 related, but all this include stuff is somewhat of a mystery to me so who knows.
Attachment #8647283 - Flags: review?(Pidgeot18)
Attachment #8643996 - Flags: review?(mh+mozilla)
Assignee: nobody → mh+mozilla
Attachment #8643996 - Attachment is obsolete: true
Attachment #8647283 - Attachment is obsolete: true
Attachment #8650884 - Flags: review?(nfroyd)
Since Char16.h is included everywhere, and MSVC 2015 uses the char16ptr_t trick
it contains, we include <string> everywhere, but that has the side effect of
breaking the build in subtle ways. One way around this would be to avoid including
Char16.h in the first place, but that requires more work than I was ready to put
in. So instead, just avoid including <string> by removing the conversion operator
for std::wstring.
Attachment #8650885 - Flags: review?(nfroyd)
Somehow, TimeStamp.cpp fails to build with MSVC 2015 without this.
Attachment #8650887 - Flags: review?(nfroyd)
Note, I think we should remove the char16ptr_t thing, and switch uses of std::wstring to std::u16string or some other type.
Fixup for OSX.
Attachment #8650884 - Attachment is obsolete: true
Attachment #8650884 - Flags: review?(nfroyd)
Attachment #8650943 - Flags: review?(nfroyd)
This fixes warnings that we are actually already getting, but that adding cmath to the STL wrappers makes a hard error because TimeStamp.cpp is built with warnings as errors.
Attachment #8650959 - Flags: review?(nfroyd)
Comment on attachment 8650885 [details] [diff] [review]
Avoid including <string> from Char16.h

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

::: mfbt/Char16.h
@@ -92,5 @@
>    operator bool() const
>    {
>      return mPtr != nullptr;
>    }
> -  operator std::wstring() const

I wonder if it's worth |= delete|'ing this, just so we have a helpful place to hang a "this is why we don't provide conversion to wstring" comment.
Attachment #8650885 - Flags: review?(nfroyd) → review+
Attachment #8650943 - Flags: review?(nfroyd) → review+
Attachment #8650887 - Flags: review?(nfroyd) → review+
Comment on attachment 8650959 [details] [diff] [review]
Avoid conflicting declarations for our raise wrappers on Windows

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

::: memory/mozalloc/msvc_raise_wrappers.h
@@ +17,5 @@
>  
>  #include "mozilla/mozalloc_abort.h"
>  
> +// xutility will declare those functions in the std namespace, which we
> +// will define in msvc_raise_wrappers.cpp

This comment reads a little funny to me (mostly around "those functions", how about:

"xutility will declare the following functions in the std namespace.  We #define them to be named differently so we can ensure the exception throwing semantics of these functions work exactly the way we want, by defining our own versions in msvc_raise_wrappers.cpp."
Attachment #8650959 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #18)
> Comment on attachment 8650885 [details] [diff] [review]
> Avoid including <string> from Char16.h
> 
> Review of attachment 8650885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Char16.h
> @@ -92,5 @@
> >    operator bool() const
> >    {
> >      return mPtr != nullptr;
> >    }
> > -  operator std::wstring() const
> 
> I wonder if it's worth |= delete|'ing this, just so we have a helpful place
> to hang a "this is why we don't provide conversion to wstring" comment.

How do you = delete it without including <string> for the definition of wstring? The problem being with the include, not wstring.
(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #18)
> > I wonder if it's worth |= delete|'ing this, just so we have a helpful place
> > to hang a "this is why we don't provide conversion to wstring" comment.
> 
> How do you = delete it without including <string> for the definition of
> wstring? The problem being with the include, not wstring.

Fair point!  Can't even forward-declare wstring, since it's a typedef...
You need to log in before you can comment on or make changes to this bug.