Closed
Bug 598615
Opened 14 years ago
Closed 10 years ago
Rename HAVE_64BIT_OS to something more appropriate
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla33
People
(Reporter: jimm, Assigned: bbondy)
References
Details
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
HAVE_64BIT_OS indicates we're building a 64-bit version, but "have 64bit os" is an awful way of communicating this.
I'd suggest something like "_USING64BITCL" or "USING_64BIT_CL".
Also, adding a corresponding 32 bit flag would be helpful. We have cases where we're #ifndef HAVE_64BIT_OS for 32-bit, which seems wrong.
The number of changes at this point is pretty small, at least for m-c:
http://mxr.mozilla.org/mozilla-central/search?string=HAVE_64BIT_OS
Comment 1•14 years ago
|
||
I feel like maybe we should ditch this entirely and just use explicit CPU checks. Our only Tier-1 64-bit platform is x86-64 anyway (I think Debian builds for ia64 and maybe some other weirdo 64-bit CPUs, but that's less important.)
Reporter | ||
Comment 2•14 years ago
|
||
(In reply to comment #1)
> I feel like maybe we should ditch this entirely and just use explicit CPU
> checks. Our only Tier-1 64-bit platform is x86-64 anyway (I think Debian builds
> for ia64 and maybe some other weirdo 64-bit CPUs, but that's less important.)
http://mxr.mozilla.org/mozilla-central/source/configure.in#2488
Ted, are these the defines you're referring to? I find the reference to AMD for x86_64-* here misleading.
Why? AMD64 is what the architecture is generally referred to because it was invented by AMD.
Comment 4•14 years ago
|
||
No, I meant the ones that are defined by the compiler, like for MSVC:
http://msdn.microsoft.com/en-us/library/b0084kay.aspx
_M_IX86 and _M_X64
or for GCC, __i386__ and __x86_64__.
Reporter | ||
Comment 5•14 years ago
|
||
(In reply to comment #4)
> No, I meant the ones that are defined by the compiler, like for MSVC:
> http://msdn.microsoft.com/en-us/library/b0084kay.aspx
>
> _M_IX86 and _M_X64
>
> or for GCC, __i386__ and __x86_64__.
Would those be handled by our preprocessor when building things like the installer? (re bug 597911)
Comment 6•14 years ago
|
||
No, we'd have to add them to DEFINES via the build system if you want to use them there.
Comment 7•14 years ago
|
||
Why would you ever bother platform-specific defines, when the sizeof(void*)==8 check is exactly, by definition, what you want?
But the name of this macro (and the message in the configure check) really need to get changed as you say!
Assignee | ||
Comment 8•10 years ago
|
||
The check we're using with CONFIGURE_STATIC_ASSERT(sizeof(void*) == 8) I think is correct for what we want. Also having such a define is nice and clean.
I'm going to rename this to HAVE_64BIT_BUILD and request review to close this out. If anyone disagrees with this approach please let me know in this bug within the next day.
Assignee: nobody → netzen
Comment 9•10 years ago
|
||
I was prepared to bikeshed the naming, but your proposal is better than what's there now, and any suggestions I have would just be nitpicky.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8444876 -
Flags: review?(ted)
Assignee | ||
Comment 11•10 years ago
|
||
$ grep HAVE_64BIT_OS -R . --exclude="./.hg/*"
$
Updated•10 years ago
|
Attachment #8444876 -
Flags: review?(ted) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Target Milestone: --- → mozilla33
Comment 13•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•