Remove mozilla/StandardInteger.h

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

6 years ago
All of our supported compilers should have <stdint.h> now.
Assignee

Comment 1

6 years ago
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #782709 - Flags: review?(jwalden+bmo)
Assignee

Comment 2

6 years ago
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+
Assignee

Comment 6

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

Comment 7

6 years ago
(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. :(
Assignee

Comment 8

6 years ago
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)
Assignee

Comment 9

6 years ago
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.
Assignee

Comment 11

6 years ago
Attachment #782793 - Flags: review?(jwalden+bmo)
Assignee

Comment 12

6 years ago
(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+
Assignee

Comment 15

6 years ago
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!

Comment 19

6 years ago
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.