Closed Bug 472622 Opened 16 years ago Closed 16 years ago

prmjtime.cpp failed to compile on SunOS 5.*

Categories

(Core :: JavaScript Engine, defect)

All
OpenSolaris
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
copying "#if defined" from nsprpub/pr/src/misc/prtime.c
Assignee: general → ginn.chen
Attachment #355922 - Flags: review?(crowder)
What exactly are the build-errors we get here? Why does this need to be based on OS defines? Why can't we have nice things? I'm cc:ing Jim and Ted for thoughts on the proper, configure-y way to do this.
OS/2 fails as well: E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp: In function `size_t PRMJ_FormatTime(char*, int, const char*, PRMJTime*)': E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:612: error: 'struct tm' has no member named 'tm_gmtoff' E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:612: error: 'struct tm' has no member named 'tm_gmtoff' E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:613: error: 'struct tm' has no member named 'tm_zone' E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:613: error: 'struct tm' has no member named 'tm_zone'
An AC_TRY_COMPILE test could decide whether those structure members were present. Ginn, do you want to take a shot at it? Assign the bug to me if you'd rather I did it.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #355922 - Attachment is obsolete: true
Attachment #356466 - Flags: review?(crowder)
Attachment #355922 - Flags: review?(crowder)
Comment on attachment 356466 [details] [diff] [review] patch v2 Should this > #ifdef HAVE_LOCALTIME_R > { > struct tm td; > time_t bogus = 0; > localtime_r(&bogus, &td); >+#ifdef HAVE_TM_ZONE > a.tm_gmtoff = td.tm_gmtoff; > a.tm_zone = td.tm_zone; >+#endif > } > #endif not better be #if defined(HAVE_LOCALTIME_R) && defined(HAVE_TM_ZONE) { struct tm td; time_t bogus = 0; localtime_r(&bogus, &td); a.tm_gmtoff = td.tm_gmtoff; a.tm_zone = td.tm_zone; } #endif It seems to me that it doesn't really make sense to compile the first three lines but not the last two.
Comment on attachment 356466 [details] [diff] [review] patch v2 Awesome, thanks. Can you land, or does this need help?
Attachment #356466 - Flags: review?(crowder) → review+
Comment on attachment 356466 [details] [diff] [review] patch v2 Actually, yes, I'm in agreement with comment #6. Thanks.
Attachment #356466 - Flags: review+ → review-
(In reply to comment #5) > Created an attachment (id=356466) [details] > patch v2 Hi, Ginn --- the configure changes look good. Mozilla can't use autoconf compile tests on Windows, because it doesn't know how to invoke the Visual C++ compiler properly. So all tests that do compilation need to take place inside one of the 'if test -z "$SKIP_COMPILER_CHECKS"' blocks that are already present in the configure script. In some patches I haven't landed yet, I've taken to hard-coding answers for Windows in the block that goes like this: if test -n "$_WIN32_MSVC"; then SKIP_PATH_CHECKS=1 SKIP_COMPILER_CHECKS=1 SKIP_LIBRARY_CHECKS=1 fi ... my thinking being that, since this is where we say that we can't run the tests, it's the logical place to handle the alternative. (I have some ideas on how to support MSVC in configure, but I haven't tried them out yet.) Some autoconf style points: - Use AC_CACHE_CHECK --- you've basically written out its definition. - Use 'yes' and 'no' for autoconf values, not 'true' and 'false'. - It might be nice if the cache variable name included 'gmtoff'.
Attached patch patch v3 (obsolete) — Splinter Review
Thanks for the comments. I'm a newbie to autoconf.
Attachment #356466 - Attachment is obsolete: true
Attachment #356706 - Flags: review?(jim)
Attachment #356706 - Flags: review?(crowder)
Attachment #356706 - Flags: review?(crowder) → review+
Attachment #356706 - Flags: review?(jim) → review-
Comment on attachment 356706 [details] [diff] [review] patch v3 The autoconf code looks perfect now --- thanks! However, since you use AC_TRY_COMPILE, this needs to be inside one of the SKIP_COMPILER_CHECKS blocks. For example, up about 40 lines or so in js/src/configure.in from the place this patch inserts your code you'll see something that says: fi # SKIP_COMPILER_CHECKS that marks the end of a long block of checks that use AC_TRY_COMPILE, AC_TRY_RUN, and things like that. Your block, comments and all, needs to inside that 'if', perhaps as the next sibling of the block that starts: dnl ======================================================== dnl Autoconf test for gcc 2.7.2.x (and maybe others?) so that we don't dnl provide non-const forms of the operator== for comparing nsCOMPtrs to dnl raw pointers in nsCOMPtr.h. (VC++ has the same bug.) dnl ========================================================
Attached patch patch v4Splinter Review
Attachment #356706 - Attachment is obsolete: true
Attachment #356904 - Flags: review?(jim)
Comment on attachment 356904 [details] [diff] [review] patch v4 Looks great!
Attachment #356904 - Flags: review?(jim) → review+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 411726
No longer depends on: 411726
Keywords: regression
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: