1298337 - Replace "RemoveXXX" series of type traits with std version in current codebase

Status: RESOLVED DUPLICATE of bug 1625138

(Core :: MFBT, defect)

Description

[James Cheng[:JamesCheng]]

For Bug 1277372 - Remove TypeTraits.h in place of <type_traits>

This bug is trying to remove 

RemoveCV
RemoveConst
RemoveVolatile
RemoveReference
RemoveExtent
RemovePointer

in our code base.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1298337 - Replace RemoveXXX series of type traits with std version in current codebase.

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

Comment 2

[James Cheng[:JamesCheng]]

Hi Nathan,

I have four questions need your help.

When I modify Assersions.h, I encounter the build errors like

 1:10.10 In file included from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:335:0,
 1:10.10                  from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/NotNull.h:65,
 1:10.10                  from /media/hdd/Projects/gecko-mozilla/xpcom/typelib/xpt/xpt_xdr.h:14,
 1:10.10                  from /media/hdd/Projects/gecko-mozilla/xpcom/typelib/xpt/xpt_struct.cpp:8,
 1:10.10                  from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/xpcom/typelib/xpt/Unified_cpp_xpcom_typelib_xpt0.cpp:11:
 1:10.10 /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/stl_wrappers/type_traits:54:6: error: #error "STL code can only be used with infallible ::operator new()"
 1:10.10  #    error "STL code can only be used with infallible ::operator new()"
 1:10.10       ^
 1:10.14 libdevtools_server.a.desc
 1:10.68 
 1:10.68 In the directory  /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/xpcom/typelib/xpt
 1:10.68 The following command failed to execute properly:

Q1: I don't know how to deal with that(even the root cause), Is it be inevitable or anything that I can try?

Above error also happened when I modified Move.h

3:41.87 In file included from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Move.h:12:0,
 3:41.87                  from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EnumeratedArray.h:13,
 3:41.87                  from /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ArrayUtils.h:23,
 3:41.87                  from /media/hdd/Projects/gecko-mozilla/toolkit/mozapps/update/tests/TestAUSReadStrings.cpp:34:
 3:41.87 /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/stl_wrappers/type_traits:54:6: error: #error "STL code can only be used with infallible ::operator new()"
 3:41.87  #    error "STL code can only be used with infallible ::operator new()"
 3:41.87       ^
 3:41.87 
 3:41.87 In the directory  /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/toolkit/mozapps/update/tests
 3:41.88 The following command failed to execute properly:

Q2: Besides Traits,  Should we also get rid of Move and Forward and replace it with std version?
If so, I think current patch do not need to modify Move.h and open a bug for removing it.

Q3: Once I fixed above errors, should I delete "RemoveXXX" in TypeTraits.h in this bug (I prefer to do that)?

Q4: If the answer for Q3 is YES, I would need to include <type_traits> in TypeTraits.h since IsIntegral and some traits still using "RemoveXXX", I need to replace it with std version also. And remove the test code from TestTypeTraits.cpp. Does those make sense?


Thank you very much.

Comment 3

[James Cheng[:JamesCheng]]

Attach the treeherder result to make sure all platform can compile the current patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02251540c3b0

Hope it works.

Comment 4

[James Cheng[:JamesCheng]]

It can be compiled on my local Linux machine but it seems it builds fail on all try server platforms...I need to dig out why...

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to James Cheng[:JamesCheng] from comment #2)
> When I modify Assersions.h, I encounter the build errors like
> 
>  1:10.10 In file included from
> /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/
> mozilla/Assertions.h:335:0,
>  1:10.10                  from
> /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/include/
> mozilla/NotNull.h:65,
>  1:10.10                  from
> /media/hdd/Projects/gecko-mozilla/xpcom/typelib/xpt/xpt_xdr.h:14,
>  1:10.10                  from
> /media/hdd/Projects/gecko-mozilla/xpcom/typelib/xpt/xpt_struct.cpp:8,
>  1:10.10                  from
> /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/xpcom/typelib/xpt/
> Unified_cpp_xpcom_typelib_xpt0.cpp:11:
>  1:10.10
> /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/dist/stl_wrappers/
> type_traits:54:6: error: #error "STL code can only be used with infallible
> ::operator new()"
>  1:10.10  #    error "STL code can only be used with infallible ::operator
> new()"
>  1:10.10       ^
>  1:10.14 libdevtools_server.a.desc
>  1:10.68 
>  1:10.68 In the directory 
> /media/hdd/Projects/gecko-mozilla/obj-x86_64-pc-linux-gnu/xpcom/typelib/xpt
>  1:10.68 The following command failed to execute properly:
> 
> Q1: I don't know how to deal with that(even the root cause), Is it be
> inevitable or anything that I can try?

This error is coming from:

https://dxr.mozilla.org/mozilla-central/source/config/gcc-stl-wrapper.template.h#48
https://dxr.mozilla.org/mozilla-central/source/config/msvc-stl-wrapper.template.h#67

which is assuming that all STL headers that we wrap must call allocation functions.  This isn't a good assumption.  The easiest way around this would be to whitelist type_traits there so that we didn't produce that error, but I'm not sure offhand if that's the best way.  Another way would be to remove type_traits from our list of wrapped STL headers.  Would you please file a bug on this problem in Core::Build Config and CC glandium?

> Q2: Besides Traits,  Should we also get rid of Move and Forward and replace
> it with std version?
> If so, I think current patch do not need to modify Move.h and open a bug for
> removing it.

Yes, we should open a bug on this.

> Q3: Once I fixed above errors, should I delete "RemoveXXX" in TypeTraits.h
> in this bug (I prefer to do that)?

Yes.

> Q4: If the answer for Q3 is YES, I would need to include <type_traits> in
> TypeTraits.h since IsIntegral and some traits still using "RemoveXXX", I
> need to replace it with std version also. And remove the test code from
> TestTypeTraits.cpp. Does those make sense?

That makes sense.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8785222 [details]
Bug 1298337 - Replace RemoveXXX series of type traits with std version in current codebase.

https://reviewboard.mozilla.org/r/74496/#review72468

This patch generally looks fine.  I'm cancelling the review because there are various small things to fix, and it sounds like you were also going to remove things from `TypeTraits.h` and the tests, plus the patch needs to build on try.  I'll just review that patch when it comes.  Thank you for working on this!

::: dom/media/MediaEventSource.h:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef MediaEventSource_h_
>  #define MediaEventSource_h_
>  
> +#include <type_traits>

Please move this after the other includes.

::: dom/media/gmp/GMPDecryptorChild.cpp:6
(Diff revision 1)
> +#include <ctime>
> +#include <type_traits>

Please don't move these up here; GMPDecryptorChild.cpp follows a common pattern where the header (GMPDecryptorChild.h) is included first to ensure that it doesn't silently depend on other headers.  Put the include of `type_traits` down with the original include of `ctime`, please.

::: ipc/glue/ProtocolUtils.cpp:27
(Diff revision 1)
>  #define TARGET_SANDBOX_EXPORTS
>  #include "mozilla/sandboxTarget.h"
>  #endif
>  
>  #if defined(MOZ_CRASHREPORTER) && defined(XP_WIN)
> +#include <type_traits>

Please move this back to where the `TypeTraits.h` include was.

::: js/public/HashTable.h:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef js_HashTable_h
>  #define js_HashTable_h
>  
> +#include <type_traits>

`js/` has its own specific rules about how to order includes, as you may have seen from your try push failures.  You'll have to put `type_traits` in the right place according to that ordering, for this change and all other changes in `js/`.

::: mfbt/Maybe.h:12
(Diff revision 1)
> +#include <new>  // for placement new
> +#include <type_traits>

Please don't move these.

::: mfbt/Pair.h:12
(Diff revision 1)
>  /* A class holding a pair of objects that tries to conserve storage space. */
>  
>  #ifndef mozilla_Pair_h
>  #define mozilla_Pair_h
>  
> +#include <type_traits>

MFBT is a little inconsistent in the ordering of its includes, but let's try to put new `<`-bracketed includes after `mozilla/` includes, please.

::: mfbt/UniquePtr.h:12
(Diff revision 1)
>  /* Smart pointer managing sole ownership of a resource. */
>  
>  #ifndef mozilla_UniquePtr_h
>  #define mozilla_UniquePtr_h
>  
> +#include <type_traits>

Here too.

::: mfbt/Variant.h:11
(Diff revision 1)
>  
>  /* A template class for tagged unions. */
>  
>  #include <new>
>  #include <stdint.h>
> +#include <type_traits>

This one is fine; it follows existing style.

::: xpcom/glue/nsTArray.h:11
(Diff revision 1)
>  
>  #ifndef nsTArray_h__
>  #define nsTArray_h__
>  
> +#include <new>
> +#include <type_traits>

Please move this down to where `<new>` is included.

::: xpcom/glue/nsThreadUtils.h:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef nsThreadUtils_h__
>  #define nsThreadUtils_h__
>  
> +#include <type_traits>

Please move this after the other includes.

Comment 7

[James Cheng[:JamesCheng]]

Sorry I don't know the including convention in MFBT,

So I make lots of include ordering changes, I just follow the coding guideline from MDN[1]....

I will follow your instruction and hope the try build on Windows and Android can pass the build.

Since Windows and Android build error is totally different from Linux(caused by check_spidermonkey.py), it may take more time to see what happened.


Thank you very much :)


[1]
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

Includes are split into three blocks and are sorted alphabetically in each block:
The main header: Foo.h in Foo.cpp
Standard library includes: #include <map>
Mozilla includes: #include "mozilla/dom/Element.h"

Comment 8

[James Cheng[:JamesCheng]]

Attachment: Reordering-the-include-header.patch

Comment 9

[James Cheng[:JamesCheng]]

Attachment: Put-include-new-before-type_traits-no-reason.patch

Comment 10

[James Cheng[:JamesCheng]]

Hello Nathan,
Sorry that I was still stuck on the compile error problems that should need your help.

I suffered from that for the weekend and cannot have any ideas.

Please see below questions,

I used local check_spidermonkey.py and fixed all the check errors as attachment 8785745 [details] [diff] [review] shown(I will remove the comments if I overcame all problems) and can build it on my local Linux and try server machines.

It seems JS include rule is
#include "mozilla/xxx"
#include <stdxxx>
#include others...

But I am trying to build it on my local Windows&Android machine but failed.

[On Windows]
First I met lots of syntax errors as [1] shown. I have no idea why it came and tried to google it for solution.

I just can find an ancient bug on bugzilla [2] that had similar compile errors and I was inspired by this bug...so I just add a
#include <new> before #include <type_traits> as shown in attachment 8785746 [details] [diff] [review] . It makes a lot of progress but still have another problem.

Is there some mysterious internal MSVC bugs that I met? But I cannot reproduce it by some test sample...

Second, once I add |#include <new> before #include <type_traits>|
I got unresolved external symbol when in linking stage as shown on [3][4].

The root cause seems to be similar to Android case [5]...

They cannot find the definition of moz_xmalloc.

I expect that the error will be something like |cannot find the declaration| but it showed link error...


Could you please give me some suggestion that I can keep moving on this bug? I was frustrated and ask myself am I qualified of working on MFBT?

Thank you very much.


[1] MSVC Syntax Error
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f7673ae502e&selectedJob=26480558

[2] Bug 602558 - MSVC10 fails to compile with Google OTS source
https://bugzilla.mozilla.org/show_bug.cgi?id=602558

[3] MSVC Link Error
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd1668ff132&selectedJob=26481237

[4] Detail about [3]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd1668ff132&selectedJob=26481239

15:56:56     INFO -  Executing: ..\..\..\..\..\vs2015u2\VC\bin\amd64_x86\link.exe -nologo -out:TestAUSHelper.exe -pdb:TestAUSHelper.pdb @c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\toolkit\mozapps\update\tests\tmpmexlvg.list -SUBSYSTEM:CONSOLE,5.01 -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib crypt32.lib wintrust.lib shlwapi.lib
15:56:56     INFO -  c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\toolkit\mozapps\update\tests\tmpmexlvg.list:
15:56:56     INFO -      TestAUSHelper.obj
15:56:56     INFO -      ..\common-standalone\certificatecheck.obj
15:56:56     INFO -      ..\common-standalone\pathhash.obj
15:56:56     INFO -      ..\common-standalone\readstrings.obj
15:56:56     INFO -      ..\common-standalone\registrycertificates.obj
15:56:56     INFO -      ..\common-standalone\uachelper.obj
15:56:56     INFO -      ..\common-standalone\updatehelper.obj
15:56:56     INFO -      ..\common-standalone\updatelogging.obj
15:56:56     INFO -  updatehelper.obj : error LNK2019: unresolved external symbol __imp__moz_xmalloc referenced in function "class mozilla::UniquePtr<char [0],class mozilla::DefaultDelete<char [0]> > __cdecl mozilla::MakeUnique<char [0]>(unsigned int)" (??$MakeUnique@$$BY0A@D@mozilla@@YA?AV?$UniquePtr@$$BY0A@DV?$DefaultDelete@$$BY0A@D@mozilla@@@0@I@Z)
15:56:56     INFO -  TestAUSHelper.exe : fatal error LNK1120: 1 unresolved externals
15:56:56     INFO -  c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/config/rules.mk:729: recipe for target 'TestAUSHelper.exe' failed
15:56:56     INFO -  mozmake.EXE[5]: *** [TestAUSHelper.exe] Error 1120
15:56:56     INFO -  mozmake.EXE[5]: Leaving directory 'c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/obj-firefox/toolkit/mozapps/update/tests'
15:56:56     INFO -  c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/config/recurse.mk:71: recipe for target 'toolkit/mozapps/update/tests/target' failed
15:56:56     INFO -  mozmake.EXE[4]: *** [toolkit/mozapps/update/tests/target] Error 2
15:56:56     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....


[5] Error on Android Build
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd1668ff132&selectedJob=26481101

Comment 11

[Nathan Froyd [:froydnj]]

I don't know what's going on with the MSVC syntax errors.  Those are just bizarre.

The MSVC and Android link errors are presumably because including <new> in various places causes things to use our infallible operator new (which uses moz_xmalloc) rather than the standard library's operator new (which simply uses malloc).  I think removing type_traits from the list of STL headers we wrap is the way forward here:

https://dxr.mozilla.org/mozilla-central/source/config/stl-headers#37

I believe that should clear up the problems you're seeing.

Comment 12

[James Cheng[:JamesCheng]]

Hi Nathan,

Just want to let you know the experimental result.

After removing the type_traits from stl-headers

The try result is

[ attachment 8785745 [details] [diff] [review] ]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1062fa6e7722&selectedJob=26555228

c:\builds\moz2_slave\try-w64-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla\msvc_raise_wrappers.h(12): fatal error C1189: #error: "Unable to wrap _RAISE(); CRT _RAISE() already defined"
mozmake.EXE[5]: *** [Unified_cpp_systemservices0.obj] Error 2
mozmake.EXE[4]: *** [dom/media/systemservices/target] Error 2

The progress is that it can pass the Android Build.

[attachment 8785745 [details] [diff] [review] + attachment 8785746 [details] [diff] [review]]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4da015fd7ef&selectedJob=26555335
Still got unresolved external symbol error.


[Problem]

#error: "Unable to wrap _RAISE(); CRT _RAISE() already defined" occurred by

https://dxr.mozilla.org/mozilla-central/source/config/msvc-stl-wrapper.template.h#25hhe

I have no idea about this error I guess maybe bug 1298566 is related to this error also.

I will keep track with that bug.


Thank you for your kindly help.

Comment 13

[Nathan Froyd [:froydnj]]

I think Mike has already responded in bug 1298566 to this concern.  Sorry this is turning out to be such a rathole; please do feel free to find something else to work on if this work is turning out to be more than you have time for!

Comment 14

[James Cheng[:JamesCheng]]

unassigned me due to there are many mysterious errors that I cannot handle it.