Closed
Bug 465640
Opened 16 years ago
Closed 15 years ago
Use autoconf to declare stdint types on platforms that don't have stdint.h
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: jimb)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 4 obsolete files)
15.18 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
nanojit uses stdint.h, which still isn't available on all platforms. We don't need all the types defined in C99. I'm thinking of {u,}int{8,16,32,64,ptr}_t (and whatever else nanojit uses, if anything).
Assignee | ||
Comment 1•16 years ago
|
||
At configure time, check for <stdint.h>. If we don't have it, observe the sizes of various integer types. New header js/src/jsstdint.h includes <stdint.h> if it's available, or uses the sizes from configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself otherwise. Note that since these types are used in the public JavaScript API, the configure script needs to place the definitions it finds in js-config.h, the installed configure-generated header, so it can be used by jsapi.h and that gang.
Assignee | ||
Comment 2•16 years ago
|
||
Jason, the attached patch should give you definitions for {,u}int{8,16,32,64,ptr}_t on all platforms. Let me know if it's good enough for what you need. Oh, I think it'll make 'make check' fail because I didn't add js/src/build/autoconf/mozsizeof.m4 to build/autoconf as well. I'll have to revise that. But it shouldn't interfere with converting js/src to use the new types.
Comment 3•16 years ago
|
||
Comment 1 makes the point that API users have been given public typedefs such as jsint, uint32, etc., for ever, via jsapi.h. We should not break API compat there, even if we make our internal code use the stdint names. /be
Reporter | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) Agreed. This patch doesn't (and shouldn't) delete any types or redefine any existing types.
Updated•16 years ago
|
Assignee: general → nobody
Component: JavaScript Engine → Build Config
QA Contact: general → build-config
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → jorendorff
Comment 5•16 years ago
|
||
Is this patch ready for review?
Reporter | ||
Comment 6•16 years ago
|
||
(In reply to comment #2) > Jason, the attached patch should give you definitions for > {,u}int{8,16,32,64,ptr}_t on all platforms. Let me know if it's good enough > for what you need. Yes, it's good enough!
Reporter | ||
Comment 7•16 years ago
|
||
Jim, please feel free to request review.
Assignee: jorendorff → jim
Assignee | ||
Comment 8•16 years ago
|
||
[Revised as suggested in comment 2.] At configure time, check for <stdint.h>. If we don't have it, observe the sizes of various integer types. New header js/src/jsstdint.h includes <stdint.h> if it's available, or uses the sizes from configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself otherwise. Note that since these types are used in the public JavaScript API, the configure script needs to place the definitions it finds in js-config.h, the installed configure-generated header, so it can be used by jsapi.h and that gang.
Attachment #349057 -
Attachment is obsolete: true
Attachment #353247 -
Flags: review?(crowder)
Comment 9•16 years ago
|
||
Comment on attachment 353247 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h This looks good to me; though, I've not tried building with it on WinCE yet. Also, ted or bsmedberg should probably look more closely as the AC/M4 stuff.
Attachment #353247 -
Flags: review?(crowder) → review+
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 353247 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h Hi, Ted. How does the Autoconf stuff look?
Attachment #353247 -
Flags: review+ → review?(ted.mielczarek)
Reporter | ||
Comment 11•16 years ago
|
||
Jim, if you use the "Add'l Review" field (on the attachment details page) to request another review, it won't erase the previous r+.
Updated•16 years ago
|
Attachment #353247 -
Flags: review+
Assignee | ||
Comment 12•16 years ago
|
||
Ah, okay --- thanks!
Comment 13•16 years ago
|
||
Comment on attachment 353247 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h +dnl The Initial Developer of the Original Code is +dnl Jim Blandy +dnl Portions created by the Initial Developer are Copyright (C) 2008 +dnl the Initial Developer. All Rights Reserved. +dnl +dnl Contributor(s): I believe standard practice is to list "The Mozilla Foundation" as original author (since they hold copyright on our work), and yourself under contributors. Overall this looks good, although I will admit that I'm not an m4 expert. (is anyone?) I do wonder if you could tie this in using the configure_static_assert macros (shorthand for the ternary-operator-in-array-size-typedef trick you're using here), but it may not be worth the effort, and there's already a configure test at that point to ensure that the trick works (we error out if it doesn't). My only qualm is that it feels like the #error conditions in jsstdint.h ought to be happening in configure. If your system isn't going to compile our code, it ought to error in configure, not in a random header file. Do you think you could rework it to have those conditions error out in configure instead?
Assignee | ||
Comment 14•16 years ago
|
||
IRC conversation: configure should fail, and the #error directives should stay in jsstdint.h as sanity checks.
Assignee | ||
Comment 15•16 years ago
|
||
The CONFIGURE_STATIC_ASSERT stuff is headed in the right direction, in that there's no reason to be writing out the array magic each time, and in trying to ensure some decent error messages result from it. But the way it's done in configure.in makes it hard to use reliably from different .m4 files. The setup should be pulled out into its own M4 macro, and AC_REQUIRE/AC_PROVIDE used to make sure everything gets run. Or, alternatively, the whole thing might be able to be converted into an M4 macro that expands to the C code. There's a slim chance it might even be able to generate configure.in line numbers, if I remember right. But I don't think this is worth doing right now.
Assignee | ||
Comment 16•16 years ago
|
||
[Revised to error out at configure time if we can't find the types we expect.] At configure time, check for <stdint.h>. If we don't have it, observe the sizes of various integer types. New header js/src/jsstdint.h includes <stdint.h> if it's available, or uses the sizes from configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself otherwise. Note that since these types are used in the public JavaScript API, the configure script needs to place the definitions it finds in js-config.h, the installed configure-generated header, so it can be used by jsapi.h and that gang.
Attachment #353247 -
Attachment is obsolete: true
Attachment #353891 -
Flags: review?(ted.mielczarek)
Attachment #353247 -
Flags: review?(ted.mielczarek)
Updated•16 years ago
|
Attachment #353891 -
Flags: review?(ted.mielczarek) → review+
Comment 17•16 years ago
|
||
Comment on attachment 353891 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h Looks good, thanks!
Assignee | ||
Comment 18•16 years ago
|
||
Comment on attachment 353891 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h Sorry, guys. This is totally borked on Windows. Suggest we obsolete that platform.
Attachment #353891 -
Attachment is obsolete: true
Assignee | ||
Comment 19•15 years ago
|
||
[Ben --- note especially the way the patch deals with Windows, hard-coding answers to questions configure is expected to answer. If we can't actually run compilation tests in configure, I'm not sure what else would be better. Comments welcome.] At configure time, check for <stdint.h>. If we don't have it, observe the sizes of various integer types. On Windows, where we can't run compilation tests in configure, hard-code definitions suggesting the use of the built-in __intN types and <stddef.h> for the pointer-sized types. Note that since these types are used in the public JavaScript API, the configure script needs to place the definitions it finds in js-config.h, the installed configure-generated header, so it can be used by jsapi.h and that gang. New header js/src/jsstdint.h does what it takes to get the right definitions. It includes <stdint.h> if it's available, uses the sizes from configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself, or uses the __intN types and the <stddef.h> header. Remove now-extraneous and possibly conflicting definitions of intN_t types from js/src/nanojit/avmplus.h.
Attachment #355830 -
Flags: review?(benjamin)
Comment 20•15 years ago
|
||
Now that we have these standard names, can we get rid of jscpucfg? See bug 269538, bug 471059, bug 470071. I *think* the only thing we don't have is stack-grown-direction... and perhaps we can just hard-code the OS/CPUs that have upwards growth?
Assignee | ||
Comment 21•15 years ago
|
||
I think getting rid of jscpucfg is covered by bug 467453.
Comment 22•15 years ago
|
||
(In reply to comment #20) > Now that we have these standard names, can we get rid of jscpucfg? See bug > 269538, bug 471059, bug 470071. I *think* the only thing we don't have is > stack-grown-direction... and perhaps we can just hard-code the OS/CPUs that > have upwards growth? Downward. But see bug 192414 on upward-stack-growth oddball architectures. /be
Assignee | ||
Comment 23•15 years ago
|
||
Ben would like the axes rotated for the tests, so configure produces things like -DJS_INT32_TYPE=int Something like MOZ_TYPE_SIZE(JS_, 32, [short, int, long int]) but not quite. "If that's not too hard, I think it's preferable"
Updated•15 years ago
|
Attachment #355830 -
Flags: review?(benjamin)
Comment 24•15 years ago
|
||
Comment on attachment 355830 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h Speaking with jimb briefly, I'd *like* to see this try along the other axis, e.g. MOZ_TYPE_SIZE(JS_, unsigned, 32, ["int", "short", "long"]);
Assignee | ||
Comment 25•15 years ago
|
||
[Revised per Ben's suggestions.] At configure time, check for <stdint.h>. If we don't have it, find integer types of various sizes. On Windows, where we can't run compilation tests in configure, hard-code definitions suggesting the use of the built-in __intN types for the exact-size types, and <stddef.h> for the pointer-sized types. Use namespace-clean names for the preprocessor macros we define. Since these types are used in the public JavaScript API, the configure script needs to place the definitions it finds in js-config.h, the installed configure-generated header, so it can be used by jsapi.h and that gang. New header js/src/jsstdint.h does what it takes to get definitions for the exact-size and pointer-size integral types. It includes <stdint.h> when available, uses the types found by configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself, or uses the __intN types and the <stddef.h> header. Remove now-unnecessary and possibly conflicting definitions of intN_t types from js/src/nanojit/avmplus.h.
Attachment #355830 -
Attachment is obsolete: true
Attachment #356204 -
Flags: review?(benjamin)
Comment 26•15 years ago
|
||
Comment on attachment 356204 [details] [diff] [review] Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h woot
Attachment #356204 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 27•15 years ago
|
||
Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/557bc4bcdc30
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 28•15 years ago
|
||
Just out of curiosity: Are there plans to use stdint types throughout the whole mozilla tree?
Updated•15 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Assignee | ||
Comment 29•15 years ago
|
||
I think it depends on how well this turns out, and how enthusiastic the maintainers of that code are. We're not going to mess with NSPR or NSS, certainly. js/src/nanojit is already using the <stdint.h> types, and js/src also seems to be very deliberate about value sizes, so it's not a hard call there.
Comment 30•15 years ago
|
||
More standard-looking code, all else equal, would be good. Some object to the _t suffix (I'm an old kernel hacker, so used to it). But really, changing the window dressing is low priority. It risks messing with indentation style; it requires reviewer bandwidth. It's not automatable. Mozilla has never imposed a single, detailed code style on all original code, and we won't start. Using standard types is generally winning, although typedefs have their place too. I'm just happy we have stdint.h support in 2009 -- we thought it would have to wait for the next decade. /be
Comment 31•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6edbb7b663b3 Pushed to 1.9.1 as a part of the sequence of patches for bug 470071
Keywords: fixed1.9.1
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
•