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)
29.23 KB,
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•13 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
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57852745f33b
Target Milestone: --- → mozilla33
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57852745f33b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•