Stop including prtypes.h for endian macros

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Blocks: 1 bug)

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
We don't have to pull in all prtype junks only for endian macros.

Comment 1

5 years ago
We should just use MOZ_LITTLE_ENDIAN and MOZ_BIG_ENDIAN from mozilla/Endian.h instead of the NSPR equivalents, I think.
(Assignee)

Comment 2

5 years ago
Oh, I din't know that.
Summary: Replace prtypes.h with prcpucfg.h if possible → Stop including prtypes.h for endian macros
(Assignee)

Comment 3

5 years ago
Unfortunately MFBT is unusable in qcms because qcms is C code...

Comment 4

5 years ago
(In reply to comment #3)
> Unfortunately MFBT is unusable in qcms because qcms is C code...

By doing some #ifdef __cplusplus stuff in Endian.h, you can make the part which defines MOZ_LITTLE_ENDIAN and MOZ_BIG_ENDIAN work in C code.  Please consider doing that if possible.  Thanks so much!
(Assignee)

Comment 5

5 years ago
Created attachment 8358759 [details] [diff] [review]
Make MFBT endian macros usable from C code
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8358759 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

5 years ago
Created attachment 8358761 [details] [diff] [review]
Stop including prtypes.h from gfx/qcms/

Looks like qcms is the only instance.
Attachment #8358761 - Flags: review?(jmuizelaar)
Attachment #8358761 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8358759 [details] [diff] [review]
Make MFBT endian macros usable from C code

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

So Jeff now tells me on IRC that he'd rather just change qcms for this, actually, than have a dependency on mfbt.  So this isn't necessary/desirable, as we'd really rather not have to think about the C/C++ distinction whenever we can get away with it.
Attachment #8358759 - Flags: review?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Comment on attachment 8358759 [details] [diff] [review]
> Make MFBT endian macros usable from C code
> 
> Review of attachment 8358759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So Jeff now tells me on IRC that he'd rather just change qcms for this,
> actually, than have a dependency on mfbt.  So this isn't
> necessary/desirable, as we'd really rather not have to think about the C/C++
> distinction whenever we can get away with it.

Copy & pasting the endian detection code from mfbt's Endian.h will be sufficient to replace the prtypes.h include.
(Assignee)

Comment 10

5 years ago
Created attachment 8370336 [details] [diff] [review]
Stop including prtypes.h from qcms

Copy & pasted the logic from mfbt.
Also removed MOZ_QCMS check because this code no longer depends on other mozilla codebase.
Attachment #8358759 - Attachment is obsolete: true
Attachment #8358761 - Attachment is obsolete: true
Attachment #8370336 - Flags: review?(jmuizelaar)
Comment on attachment 8370336 [details] [diff] [review]
Stop including prtypes.h from qcms

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

::: gfx/qcms/qcmstypes.h
@@ +4,1 @@
>  #include <stdint.h>

This can go, because we should get it at the bottom.

@@ -57,5 @@
> -#  include <sys/inttypes.h>
> -#else
> -#  include <stdint.h>
> -#endif
> -

We don't want to drop this hunk
Attachment #8370336 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 12

5 years ago
I just copy & pasted the "logic" (just including stdint.h everywhere) for integer types, just like the logic for endian macros.
The removed hunk was enclosed by !MOZ_QCMS block which is untested, unmaintained (since 2009), and effectively dead.

>-#if defined (_SVR4) || defined (SVR4) || defined (__OpenBSD__) || defined (_sgi) || defined (__sun) || defined (sun) || defined (__digital__)
>-#  include <inttypes.h>

Is this hunk should be removed?

>-#elif defined (_MSC_VER)
>-typedef __int8 int8_t;
>-typedef unsigned __int8 uint8_t;
>-typedef __int16 int16_t;
>-typedef unsigned __int16 uint16_t;
>-typedef __int32 int32_t;
>-typedef unsigned __int32 uint32_t;
>-typedef __int64 int64_t;
>-typedef unsigned __int64 uint64_t;
>-#ifdef _WIN64
>-typedef unsigned __int64 uintptr_t;
>-#else
>-typedef unsigned long uintptr_t;
>-#endif

If I left the |#if defined(_MSC_VER)| hunk, the compiler warns with C4142 on Windows.
(Assignee)

Comment 13

5 years ago
Created attachment 8370929 [details] [diff] [review]
Stop including prtypes.h from qcms, v3

Just removed |#ifdef MOZ_QCMS| and |#if defined (_MSC_VER)| hunks. Copy & pasting was not needed. Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=0ef3485e28fb

If this is insufficient, please teach me what exact part should be copied from mfbt, what exact part should be removed, and what exact part should be left.
Attachment #8370336 - Attachment is obsolete: true
Attachment #8370929 - Flags: review?(jmuizelaar)
I've checked in a version of this with the _MSC_VER hunk kept.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5be94737a83
Attachment #8370929 - Flags: review?(jmuizelaar)
https://hg.mozilla.org/mozilla-central/rev/b5be94737a83
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.