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)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: jimm, Assigned: bbondy)

References

Details

Attachments

(1 file)

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
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.)
(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.
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__.
(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)
No, we'd have to add them to DEFINES via the build system if you want to use them there.
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!
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
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.
Attached patch Patch v1.Splinter Review
Attachment #8444876 - Flags: review?(ted)
$ grep HAVE_64BIT_OS -R . --exclude="./.hg/*"
$
Attachment #8444876 - Flags: review?(ted) → review+
Blocks: 1030016
Blocks: 1030019
https://hg.mozilla.org/mozilla-central/rev/57852745f33b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.