Closed Bug 472622 Opened 15 years ago Closed 15 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)

See bug 411726 comment #40
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+
Pushed

http://hg.mozilla.org/mozilla-central/rev/b2f2ce787cbe
Status: NEW → RESOLVED
Closed: 15 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: