Closed Bug 872127 Opened 11 years ago Closed 11 years ago

Remove mozilla/StandardInteger.h

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(3 files)

Part 1: Remove support for MOZ_CUSTOM_STDINT_H
3.28 KB, patch
Waldo
: review+
ted
: review+
Details | Diff | Splinter Review
Part 2: Replace mozilla/StandardInteger.h with stdint.h
112.43 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Part 3: Remove MSStdInt.h
11.18 KB, patch
Waldo
: review+
Details | Diff | Splinter Review 
All of our supported compilers should have <stdint.h> now.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED

        Attachment #782709 -
        Flags: review?(jwalden+bmo)

    
    

    
  


  

        Attachment #782711 -
        Flags: review?(jwalden+bmo)

    
    

    
  
Comment on attachment 782709 [details] [diff] [review]
Part 1: Remove support for MOZ_CUSTOM_STDINT_H

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

I've been carrying along the patches in bug 730805 and bug 812218 for awhile now, hoping for the day when I could debug Windows PGO failures and land them.  Those have interactions/overlap with this, although not much.  I guess I'll deal with the fallout of these changes, whenever I find the time to do so.  :-\

::: js/public/LegacyIntTypes.h
@@ +16,5 @@
>   * BEWARE: Comity with other implementers of these types is not guaranteed.
>   *         Indeed, if you use this header and third-party code defining these
>   *         types, *expect* to encounter either compile errors or link errors,
>   *         depending how these types are used and on the order of inclusion.
> + *         It is safest to use only the JSAPI <stdint.h>-style types.

"only the <stdint.h> types", this was either weirdly worded or meaning has changed since it was written -- this wording is much clearer about what's suggested.

        Attachment #782709 -
        Flags: review?(jwalden+bmo) → review+

    
    

    
  
Comment on attachment 782711 [details] [diff] [review]
Part 2: Replace mozilla/StandardInteger.h with stdint.h

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

::: configure.in
@@ -6149,5 @@
>        AC_DEFINE(MOZ_CRASHREPORTER_INJECTOR)
>      fi
>    fi
>  fi
> -AC_DEFINE_UNQUOTED(BREAKPAD_CUSTOM_STDINT_H, "mozilla/StandardInteger.h")

Perhaps in a followup patch, but we should run by Ted just removing this whole thing.  http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/google_breakpad/common/breakpad_types.h seems to say we just don't need to define this at all any more, because only with MSVC < 10 was it necessary.

::: gfx/2d/Types.h
@@ +5,5 @@
>  
>  #ifndef MOZILLA_GFX_TYPES_H_
>  #define MOZILLA_GFX_TYPES_H_
>  
> +#include <stdint.h>

Move this into the <> section, alphabetically.

::: js/src/ctypes/CTypes.cpp
@@ +7,5 @@
>  #include "ctypes/CTypes.h"
>  
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/MemoryReporting.h"
> +#include <stdint.h>

Move this alphabetically into the <> section below.

::: js/src/gc/Heap.h
@@ +8,5 @@
>  #define gc_Heap_h
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/PodOperations.h"
> +#include <stdint.h>

Move this alphabetically into the <> section below.

::: js/src/ion/arm/Architecture-arm.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "ion/arm/Architecture-arm.h"
>  
> +#include <stdint.h>

Move this alphabetically into the <> section below.

::: js/src/ion/arm/Architecture-arm.h
@@ +6,5 @@
>  
>  #ifndef ion_arm_Architecture_arm_h
>  #define ion_arm_Architecture_arm_h
>  
> +#include <stdint.h>

Move this alphabetically into the <> section below.

::: js/src/jsapi.h
@@ +11,5 @@
>  
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/MemoryReporting.h"
>  #include "mozilla/RangedPtr.h"
> +#include <stdint.h>

Move this alphabetically into the <std*> section below.

::: js/src/jscrashreport.h
@@ +7,5 @@
>  #ifndef jscrashreport_h
>  #define jscrashreport_h
>  
>  #include "mozilla/GuardObjects.h"
> +#include <stdint.h>

Blank line before this, separating "mozilla/*" from <>

::: js/src/jsfriendapi.cpp
@@ +6,5 @@
>  
>  #include "jsfriendapi.h"
>  
>  #include "mozilla/PodOperations.h"
> +#include <stdint.h>

Blank line before this.

::: js/src/prmjtime.h
@@ +11,2 @@
>  
>  #include <time.h>

No blank line separating <> includes.

::: js/src/vm/DateTime.h
@@ +8,5 @@
>  #define vm_DateTime_h
>  
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/MathAlgorithms.h"
> +#include <stdint.h>

Blank line before this.

::: js/src/vm/ObjectImpl.h
@@ +8,5 @@
>  #define vm_ObjectImpl_h
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/GuardObjects.h"
> +#include <stdint.h>

Blank line before this.

::: js/src/vm/ThreadPool.h
@@ +11,2 @@
>  
>  #include <stddef.h>

No blank line before this.

::: js/src/yarr/PageBlock.h
@@ +34,2 @@
>  
>  #include <stddef.h>

No blank line before this.

::: js/xpconnect/src/xpcprivate.h
@@ +77,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/MemoryReporting.h"
> +#include <stdint.h>

Move this alphabetically into the <> includes below.

::: media/libcubeb/include/cubeb-stdint.h
@@ -1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -#include "mozilla/StandardInteger.h"

I find myself wondering if this file is necessary any more, if it's just including a standard header.  We should file a followup to investigate, or poke someone on IRC.

::: mfbt/BloomFilter.h
@@ +15,5 @@
>  #define mozilla_BloomFilter_h
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Likely.h"
> +#include <stdint.h>

Move to just before the <string.h> include.

::: mfbt/CheckedInt.h
@@ +13,5 @@
>  #define MOZ_CHECKEDINT_USE_MFBT
>  
>  #ifdef MOZ_CHECKEDINT_USE_MFBT
>  #  include "mozilla/Assertions.h"
> +#  include <stdint.h>

Move this above the #define MOZ_CHECKEDINT_USE_MFBT, and remove the <stdint.h> include in the #else a few lines down.

::: mfbt/Endian.h
@@ +66,5 @@
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Compiler.h"
>  #include "mozilla/DebugOnly.h"
> +#include <stdint.h>

Move alphabetically into the <> include section, so before <string.h>.

::: mfbt/EnumSet.h
@@ +9,5 @@
>  #ifndef mozilla_EnumSet_h
>  #define mozilla_EnumSet_h
>  
>  #include "mozilla/Assertions.h"
> +#include <stdint.h>

Blank line before the <stdint.h> include per mfbt/STYLE.

::: mfbt/FloatingPoint.h
@@ +11,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Casting.h"
> +#include <stdint.h>

Blank line before this #include to match mfbt/STYLE.

::: mfbt/HashFunctions.h
@@ +47,5 @@
>  #define mozilla_HashFunctions_h
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
> +#include <stdint.h>

This should go in its own #include section below mozilla/Types.h, see mfbt/STYLE.

::: mfbt/MathAlgorithms.h
@@ +9,5 @@
>  #ifndef mozilla_MathAlgorithms_h
>  #define mozilla_MathAlgorithms_h
>  
>  #include "mozilla/Assertions.h"
> +#include <stdint.h>

This should go alphabetically into the <> include section beneath it.

::: mfbt/Poison.h
@@ +12,5 @@
>  #ifndef mozilla_Poison_h
>  #define mozilla_Poison_h
>  
>  #include "mozilla/Assertions.h"
> +#include <stdint.h>

This should go in its own #include section below mozilla/Types.h, see mfbt/STYLE.

::: mfbt/SHA1.h
@@ +8,5 @@
>  
>  #ifndef mozilla_SHA1_h
>  #define mozilla_SHA1_h
>  
> +#include <stdint.h>

This should go alphabetically into the <> #include section below mozilla/Types.h.

::: mfbt/Types.h
@@ +24,3 @@
>  
>  /* Also expose size_t. */
>  #include <stddef.h>

Combine the two #include sections here into a single section:

/* Expose all <stdint.h> types and size_t. */
#include <stddef.h>
#include <stdint.h>

::: mfbt/tests/TestCasting.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/Casting.h"
> +#include <stdint.h>

Blank line before this.

::: netwerk/dns/nsIDNKitInterface.h
@@ +46,5 @@
>  #ifdef __cplusplus
>  extern "C" {
>  #endif /* __cplusplus */
>  
> +#include <stdint.h>

Uh, including this in an |extern "C"| is not such a great idea.  Move it above that, to just after the include-guard.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ -11,5 @@
>  #include <arpa/inet.h>
>  #endif
>  
>  #define SCTP_DEBUG 1
> -#define SCTP_STDINT_INCLUDE "mozilla/StandardInteger.h"

Worth a followup on this macro, too, and whether it's needed if everyone has <stdint.h>.

::: tools/trace-malloc/lib/nsTraceMalloc.h
@@ +10,2 @@
>  
>  #include <stdio.h> /* for FILE */

Put both <> includes into one include section.

::: xpcom/base/nsCycleCollector.cpp
@@ +142,5 @@
>  #include "mozilla/CondVar.h"
>  #include "mozilla/Likely.h"
>  #include "mozilla/mozPoisonWrite.h"
>  #include "mozilla/Mutex.h"
> +#include <stdint.h>

Move this to its own section, not mixed with mozilla/* includes.

::: xpcom/base/nsError.h
@@ +6,5 @@
>  #ifndef nsError_h__
>  #define nsError_h__
>  
>  #include "mozilla/Likely.h"
> +#include <stdint.h>

Move this into its own section, away from the "mozilla/*" includes.

::: xpcom/base/nscore.h
@@ +27,1 @@
>  #include "stddef.h"

Alphabetize, and make that <stddef.h> while you're here.

        Attachment #782711 -
        Flags: review?(jwalden+bmo) → review+

    
    

    
  
Another try push for a trivial quoting fix to get the Windows builds working: https://tbpl.mozilla.org/?tree=Try&rev=03b22ad61635

    
    

    
  
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 782711 [details] [diff] [review]
> Part 2: Replace mozilla/StandardInteger.h with stdint.h

Sigh, do you really want me to address all of these stylistic changes?  I basically did a search and replace across the tree for almost all of this patch. :(

    
    

    
  
Comment on attachment 782711 [details] [diff] [review]
Part 2: Replace mozilla/StandardInteger.h with stdint.h

Review request on the build system parts.

        Attachment #782711 -
        Flags: review?(ted)

    
    

    
  
Comment on attachment 782709 [details] [diff] [review]
Part 1: Remove support for MOZ_CUSTOM_STDINT_H

Review request on the build system parts.

        Attachment #782709 -
        Flags: review?(ted)

    
    

    
  
Until we have enforced style linting going on, we shouldn't be screwing up modules' local styles.  It shouldn't take that long to fix them all.  It's also worth noting that some portions of code here -- JS, for one -- have gone to some effort to have consistent ordering, very recently, and it would be a shame to break all that mere days after its landing.

    
    

    
  


  

        Attachment #782793 -
        Flags: review?(jwalden+bmo)

    
    

    
  
(In reply to comment #10)
> Until we have enforced style linting going on, we shouldn't be screwing up
> modules' local styles.  It shouldn't take that long to fix them all.  It's also
> worth noting that some portions of code here -- JS, for one -- have gone to
> some effort to have consistent ordering, very recently, and it would be a shame
> to break all that mere days after its landing.

OK.  I'll do that when I have some free time on my hands.  :-)

    
    

    
  
Comment on attachment 782793 [details] [diff] [review]
Part 3: Remove MSStdInt.h

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

More conflicts coming my way, yay.

        Attachment #782793 -
        Flags: review?(jwalden+bmo) → review+

    
    

    
  
Comment on attachment 782711 [details] [diff] [review]
Part 2: Replace mozilla/StandardInteger.h with stdint.h

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

I think you can remove most of the workarounds here, the code is all written to work fine with VC2010.

::: configure.in
@@ +6149,5 @@
>        AC_DEFINE(MOZ_CRASHREPORTER_INJECTOR)
>      fi
>    fi
>  fi
> +AC_DEFINE_UNQUOTED(BREAKPAD_CUSTOM_STDINT_H, <stdint.h>)

Just drop this, the code I wrote in Breakpad works fine without this for modern VC, and we don't support anything else here.

::: media/mtransport/objs.mk
@@ +55,5 @@
>  DEFINES += -DWIN
>  endif
>  
>  DEFINES += \
> +   -DR_PLATFORM_INT_TYPES='<stdint.h>' \

It looks like this code will cope just fine if you remove this (here, nicer.gyp and nrappkit.gyp):
http://mxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_types.h#87

::: media/webrtc/signaling/signaling.gyp
@@ +168,5 @@
>          'WEBRTC_RELATIVE_PATH',
>        	'HAVE_WEBRTC_VIDEO',
>          'HAVE_WEBRTC_VOICE',
>          'HAVE_STDLIB_H=1',
> +        'INTEGER_TYPES_H="<stdint.h>"',

If HAVE_STDINT_H is defined on Windows then this should be unnecessary:
http://mxr.mozilla.org/mozilla-central/source/netwerk/srtp/src/crypto/include/integers.h#62

@@ +222,5 @@
>              'WIN32',
>              'GIPS_VER=3480',
>              'SIPCC_BUILD',
>              'HAVE_WINSOCK2_H',
> +            'CPR_STDINT_INCLUDE=<stdint.h>'

The code works fine without this:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_types.h#45

::: netwerk/srtp/src/Makefile.in
@@ +56,5 @@
>    $(NULL)
>  
>  DEFINES += \
>    -DHAVE_STDLIB_H=1 \
> +  -DINTEGER_TYPES_H="<stdint.h>" \

Should be able to remove this as well.

        Attachment #782711 -
        Flags: review?(ted)

        Attachment #782709 -
        Flags: review?(ted) → review+

    
    

    
  
Follow-ups filed: bug 899556 and bug 899558.

    
    

    
  
Thanks for reordering the #includes in js/src/.  As Waldo said, bug 888088 was finished just this week, and bug 898274 is open to enforce the ordering, and it's
blocked by bug 880088, which has a patch awaiting review.  I appreciate it!

    
    

    
  
Pushed comm-central bustage fix:
https://hg.mozilla.org/comm-central/rev/e4c4ff49ed66

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: