Closed
Bug 472622
Opened 16 years ago
Closed 16 years ago
prmjtime.cpp failed to compile on SunOS 5.*
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
1.95 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
copying "#if defined" from nsprpub/pr/src/misc/prtime.c
Assignee: general → ginn.chen
Attachment #355922 -
Flags: review?(crowder)
Comment 2•16 years ago
|
||
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.
Comment 3•16 years ago
|
||
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'
Comment 4•16 years ago
|
||
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.
Attachment #355922 -
Attachment is obsolete: true
Attachment #356466 -
Flags: review?(crowder)
Attachment #355922 -
Flags: review?(crowder)
Comment 6•16 years ago
|
||
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 7•16 years ago
|
||
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 8•16 years ago
|
||
Comment on attachment 356466 [details] [diff] [review]
patch v2
Actually, yes, I'm in agreement with comment #6.
Thanks.
Attachment #356466 -
Flags: review+ → review-
Comment 9•16 years ago
|
||
(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'.
Assignee | ||
Comment 10•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #356706 -
Flags: review?(crowder) → review+
Updated•16 years ago
|
Attachment #356706 -
Flags: review?(jim) → review-
Comment 11•16 years ago
|
||
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 ========================================================
Assignee | ||
Comment 12•16 years ago
|
||
Attachment #356706 -
Attachment is obsolete: true
Attachment #356904 -
Flags: review?(jim)
Comment 13•16 years ago
|
||
Comment on attachment 356904 [details] [diff] [review]
patch v4
Looks great!
Attachment #356904 -
Flags: review?(jim) → review+
Assignee | ||
Comment 14•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•