303858 - too many basic types in JS, rest of Mozilla

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD; X11; amd64) KHTML/3.4.1 (like Gecko)
Build Identifier: 

When building on FreeBSD/amd64, the sources trigger A LOT of compiler warnings 
with -Wall. The attached patch fixes them all (including those silenced by 
-Wno-format). 
 
Please, incorporate into the next js-release. Thanks. 

Reproducible: Always




I'm quite certain, some part of this report will be mis-filed, but I could not 
find a better component. Sorry.

Comment 1

[Mikhail T.]

Attachment: fix all compiler warnings

These changes don't seem to break any tests....

Comment 2

[Jaime Mitchell (use [e-mail] for email)]

Comment on attachment 191942 [details] [diff] [review]
fix all compiler warnings

You can't review your own patches you must request review from the apropirate
peer

Comment 3

[Bob Clary [:bc] (inactive)]

As far as I know, uintptr_t isn't defined for VC6 and you need to handle that
case. Also, I am not sure about adding new header dependencies to inttypes.h.
Brendan or Shaver can provide you more guidance on what they expect from such a
patch. 

Comment 4

[Mikhail T.]

The patch was developed and tested on FreeBSD and I intend to add it to FreeBSD  
port of spidermonkey.  
  
It is customary for FreeBSD porters to notify vendors of the software being 
ported about such patches, so that other platforms can benefit (and the amount 
of patching required in the future is reduced). 
 
In other words, I consider my duty fulfilled. I'd be happy to answer questions 
about particular hunks and am eager to learn about real problems in it, that 
will affect FreeBSD (and, to a lesser extent, some other decent *nix), but I 
really do not care for Windows in general and MSVC in particular. 
 
That said, ptrdiff_t seems defined in jsstddef.h only in the case of XP_WIN16. 
Does MSVC on NT really define ptrdiff_t, but not intptr_t? What a perversion... 

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 191942 [details] [diff] [review]
fix all compiler warnings

This is a waste of time for both parties.  It would be nice to be able to use
intptr_t etc. on all platforms, but we are far from that dream world.

Worse, the jsinterp.c change adds initialization costs to the interpreter for
no reason other than to silence stupid gcc.  That's a bad trade-off.  Runtime
to users is worth much more than the cost of overlooking warnings.  Does your
version of *BSD really compile without any warnings?  How much did it cost to
achieve that, at what lost opportunity?

/be

Comment 6

[Brendan Eich [:brendan]]

One more comment: "fix all compiler warnings" implies "all compiler warnings" is
a unitary bug which must be fixed.  That's false, since (a) compilers often give
spurious warnings (e.g., the jsinterp.c ones); (b) some warnings are not
spurious but also not worth fixing; (c) even those worth fixing must compete
against other (usually more serious) bugs to fix.

Patching around symptoms is bad.  Patching around warnings can be good or bad. 
I would welcome a portable, tested patch to use intptr_t or whatever C99
provides on all supported platforms.

mrbkap, if you want to take this bug for that single purpose, please feel free.
 As summarized, however, this bug is bogus.  "Lots of warnings" is not a bug. 
Enough, or even just one, of a specific, bad warning that's worth fixing in
relation to other work, is a better bug.  Morphing this bug into a "use intptr
types from C99 in SpiderMonkey" bug would be an improvement.

/be

Comment 7

[Brendan Eich [:brendan]]

Also, didn't we go through a round of amd64 fixing?  Cc'ing jst.  Was this bug
reported against current cvs.mozilla.org sources, or against some old version of
SpiderMonkey?  If the latter, then it's an even greater waste of time.

/be

Comment 8

[Mikhail T.]

> Worse, the jsinterp.c change adds initialization costs to the interpreter
> for no reason other than to silence stupid gcc.

One may also view the warning as a sign, the code is too convoluted :-)

> That's a bad trade-off.

Feel free to remove any hunks you dislike from the patch. There is, however, a
value in bug-free compilations -- any newly introduced bug STANDS OUT.

If the bugs are routinely ignored, things like pointer-vs.-integer size, truly
unitialized variables (such as sp in the jsinterp.c) remain unfixed and lead to
misterious and difficult to reproduce crashes.

> Does your version of *BSD really compile without any warnings?

There is a concerted effort to eliminate them. It is gradual and different parts
of the tree conform to different level of cleanness. Once a particular source
subtree is cleaned, it is locked in that state by adding -Werror to the CFLAGS,
so that no new warning slips in unnoticed.

> How much did it cost to achieve that, at what lost opportunity?

This is considered part of QA, if you will -- *BSD's deliverable is the source
itself :-) And no cost can be spared on that.

> some warnings are not spurious but also not worth fixing

All warnings are worse fixing, because they impede future development... At the
very least, a comment should be added to the code like:

      /* gcc raises a bogus warning here, please ignore */

That said, gcc tends to be smarter than it often seems. For example, someone
disabled the format-warnings on Linux (-Wno-format) instead of fixing the printf
format lines to print ptrdiff_t variables properly.

> Patching around symptoms is bad.

I absolutely agree and tried to make sure, my patches are meaningful.

> Morphing this bug into a "use intptr types from C99 in SpiderMonkey"

What about fprintf() fixes, the va_copy vs. VA_COPY point the truly
uninitialized sp-pointer, the apparent wrongness of &ap (to achieve va_list *)?

> Was this bug reported against current cvs.mozilla.org sources, or against some old
> version of SpiderMonkey? If the latter, then it's an even greater waste of time.

CVS being a moving target, I am, of course, reporting against the latest
_released_ version of SpiderMonkey -- js-1.5-rc6a.tar.gz from June 16, 2004. If
that release is stale, *I* am not to blame for any time wasted.

I'd love a functional fresh release of the software, which can be debugged and
properly tested on its own and then used to build browsers instead of various
snapshots of js, which are bundled within their own trees... See also bug 303857.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> > Worse, the jsinterp.c change adds initialization costs to the interpreter
> > for no reason other than to silence stupid gcc.
> 
> One may also view the warning as a sign, the code is too convoluted :-)

For gcc?  Then gcc should be fixed.  For you?  Read more carefully.

> > That's a bad trade-off.
> 
> Feel free to remove any hunks you dislike from the patch. There is, however, a
> value in bug-free compilations -- any newly introduced bug STANDS OUT.

I grep make logs for new warnings routinely, screening out the bogus ones, so
this is not a problem for me.  Are you actively working on SpiderMonkey, or
actually screening for new warnings compared to the existing ones?

> If the bugs are routinely ignored, things like pointer-vs.-integer size,

Since your premise that warnings (not bugs) are routinely ignored is false, the
rest of your sentence does not follow.  But pointer vs. integer size warnings
are fixed, and if you'll check the current cvs.mozilla.org sources, you will
note that we've already taken amd64 changes.

> truly unitialized variables (such as sp in the jsinterp.c)

Where exactly is sp used without being set?

> remain unfixed and lead to
> misterious and difficult to reproduce crashes.

You are talking out of your hat here.

> > Does your version of *BSD really compile without any warnings?
> 
> There is a concerted effort to eliminate them. It is gradual and different parts
> of the tree conform to different level of cleanness. Once a particular source
> subtree is cleaned, it is locked in that state by adding -Werror to the CFLAGS,
> so that no new warning slips in unnoticed.

That's nice, but it doesn't change other projects' priorities, and since you
import code from other projects, I think you need to do a better job than just
congratulating yourself on having done your duty by posting a non-CVS-based diff
that lumps a bunch of changes into one patch.

As far as I can tell, it looks like your patch applies to RC6 or RC6a, both of
which are quite old compared to the tip of cvs.mozilla.org.

> > How much did it cost to achieve that, at what lost opportunity?
> 
> This is considered part of QA, if you will -- *BSD's deliverable is the source
> itself :-) And no cost can be spared on that.

That's nonsense, everything has a cost, nothing is cost-free or of infinite
value.  I'm done parlaying with you after reading this foolishness, unless you
have a real bug to report (such as that claim that sp is used before set in
js_Interpret -- I'm all ears for facts backing up that claim).

/be

Comment 10

[Mikhail T.]

Brendan! Kindly adjust your tone. I did not put bugs into your code -- I'm
offering you bug-fixes. Take them or leave them, but stop the abuse. It is not
my fault, that your latest release is 14 months old, and that I did not know,
you already "went through amd64 issues".

There are umpteen branches of js (including inside each browser) and it is
impossible for an outsider to sort through them. If you want help at all, you
should be more accomodating.

> > One may also view the warning as a sign, the code is too convoluted :-)

> For gcc?  Then gcc should be fixed.  For you?  Read more carefully.

Quite possibly, both (gcc and myself) should be fixed, but neither of us are
entirely clueless nor utterly broken. If the code is complex enough to confuse
both of us, than, perhaps, it would be cheaper (everything has a price, does not
it?) to fix the code once (Dijkstra would definetly disapprove of it, BTW). Just
a thought.

> That's nice, but it doesn't change other projects' priorities, and since you
> import code from other projects

Code of the ported software is not "imported". You are talking out of a wrong
hat here... FreeBSD ports (a.k.a. Open and NetBSD packages) make third-party
software compile -- from the third-party's own sources (although often with
BSD-specific patches).

This ports collection is a wonderful thing and I suggest you look at it some day.

It is customary to submit these patches to the software authors/maintainers, but
arguing with them, being abused by them (for patching the wrong version, for
lumping patches into one, etc.) is unusual...

> posting a non-CVS-based diff that lumps a bunch of changes into one patch.

It is trivial to split the patch into separate parts and to throw out, what you
already have. As far as I am concerned, these patches all do the same thing --
they address compiler warnings, so that the next person looking for a bug is not
distracted by them.

> As far as I can tell, it looks like your patch applies to RC6 or RC6a,
> both of which are quite old compared to the tip of cvs.mozilla.org.

I've already confirmed this for you several times. The reason I use RC6a is that
it THE LATEST AVAILABLE tarball of spidermonkey -- hardly my fault... The sooner
RC7 is released, the better for all...

> That's nonsense, everything has a cost, nothing is cost-free or of
> infinite

Absolutely. And it is better to review the warnings (as you claim to be doing
regularly) once and *address them in one way or the other* (as I wish you did),
than to have every new person go through them over-and-over wondering, if he
sees something new, or something long-known.

> (such as that claim that sp is used before set in js_Interpret -- I'm all
> ears for facts backing up that claim

In two places there is a "goto out" before sp is set to anything. Under the
out-label sp can be read (by SAVE_SP() macro). So if the code arrives to the
"out" label through one of those gotos, sp will contain garbage.

My patch explicitly sets sp to NULL in those two places prior to goto-ing to "out".

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Brendan! Kindly adjust your tone. I did not put bugs into your code -- I'm
> offering you bug-fixes.

No, you have been (apparently, mostly) wasting our time with fruitless, stale
warning fixes.  But see below, I'm not totally pig-headed.

> Take them or leave them, but stop the abuse.

Sorry, I shouldn't be abusive in bugs.  However, you seem to think that no cost
is too high, and that is abusive at the limit too -- which makes me grumpy.  I
hope you can see that.

> It is not my fault, that your latest release is 14 months old, and that I did
not know,
> you already "went through amd64 issues".

Whose fault is it, then?  Not mine.

Thanks for the sp = NULL fixes.  I should have seen those in your diff.  If you
read the code that calls SAVE_SP after label out:, it's all debugger-related,
and the operand stack is not set up at either of these goto out points.

So it doesn't matter that garbage is stored into a frame that is about to be
popped, bad though that is (it's a bug, of course).  Even with your fixes to set
sp = NULL, if the debugger's rt->throwHook were to return any code other than
JSTRAP_ERROR or JSTRAP_RETURN, and the script began with a try block, *and* if
the script contained no prolog bytecodes (for declarations), then the resulting
attempt to catch the exception returned by rt->throwHook would crash somewhere
on a partly uninitialized stack frame.  Again, even with your patch.

So my apologies, you haven't wasted my time (much :-P).

Morphing this bug and patching.  The patch won't fix all those old warnings, but
if you are willing to pull from cvs.mozilla.org or cvs-mirror.mozilla.org, apply
the patch I will hack up for the morphed bug, and let me know what warnings
remain, then I'll work with you to eliminate them somehow, or at worst declare
them gcc problems and publish my warning filtering script.

/be

Comment 12

[Brendan Eich [:brendan]]

D'oh, I was looking at RC6a sources!

This bug, the morphed one described by the current summary, was fixed last
December (in rev 3.155 of jsinterp.c).  The lack of a SpiderMonkey Release
Candidate tarball corresponding to that state of the source, or to a more recent
state, is a problem that we will fix with the RC7 bclary plans to make to match
Firefox 1.5.

I'm closing this morphed bug.  I'm still willing to entertain warning-fix bugs
if they are based on trunk CVS SpiderMonkey, and if they don't mix different
issues or patch blindly.

So that reminds me: it would be better to report the warnings themselves before
patching them.  Just setting sp = NULL, for instance, as I mentioned last time,
was not a fix even for the RC6 era jsinterp.c.  And the December fix (whose CVS
log message, by me, did not admit to the true bug, instead wrongly blaming gcc)
does in fact fix the full bug of trying to catch early over-recursed or
out-of-stack-space errors that I described in my last comment, third paragraph
from end.

So in fact this *was* all a waste of time.  Don't get me wrong: I'm not blaming
anyone exclusively for this "waste"; this kind of problem will happen even with
the best intentions, and it brought to light real problems not exactly to-do
with gcc warnings.

The failure to do a fresher RC, especially after Firefox 1.0, was wrong and I
will take the blame for it.

Apart from that, the attitude of "here is my (inadequate, based on shallow
reading of the code and warnings) patch, I've done my duty, take it or leave it,
we will keep patching it in FreeBSD if we have to", especially combined with
totally uneconomic zero-gcc-warning tolerance, was also wrong.

So two wrongs don't make a right, and all that.  Now that I'm done bitching
(again), I'm going to (a) shut up here; (b) promise to help Mikhail or whoever
else finds an unfixed warning to be annoying, or even a real bug, to fix such
warnings.  Mail me about any such at [e-mail], or file them one by
one or in coherent groups if it's clear they're bad/cheap-to-fix.

/be

Comment 13

[Mikhail T.]

> finds an unfixed warning to be annoying, or even a real bug, to fix such 
warnings 
 
There is a real problem affecting FreeBSD/amd64 and, possibly, other 64-bit 
systems. Although the get_java_wrapper's *declaration* was fixed to receive an 
lcjsobject instead of jint (an improvement over the js bundled with Firefox, 
for example), the *invocation* of the method in jsj_JSObject.c still casts 
handle (a 64-bit pointer) to jint (a 32-bit integer). 
 
I reported this against Firefox' source in bug 303656, but it would, probably, 
be better to fix this properly in the standalone spidermonkey and then use it 
instead of the browsers' own (perpetually outdated) snapshots. 
 
The trouble is, I can not test any fixes anyway, because the Java oji-plugin is 
not currently even being built on FreeBSD/amd64. But Greg Lewis -- the BSD Java 
maintainer -- promises to remedy this problem some time next week. 
 
Don't know about other OS-es with 64-bit JDKs (AIX, Irix?)... 
 
Finally, the current definition of lcjsobject (in jsjava.h) is rather 
imperfect. It depends on JS_BYTES_PER_LONG, but there is no set-in-stone 
relationship between sizeof(void*) and sizeof(jlong). 
 
Truly, lcjsobject should be (u)intptr_t with possible special handling for the 
few non-C99 compilers. 

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)

> I reported this against Firefox' source in bug 303656,

Sounds like that bug needs help (and not by comments in this one).  I received
your mail to Kyle, I'll followup by mail and in that bug.

> but it would, probably, 
> be better to fix this properly in the standalone spidermonkey and then use it 
> instead of the browsers' own (perpetually outdated) snapshots. 

SpiderMonkey's standalone tarball release does not include LiveConnect, AFAIK. 
It should not.

> Don't know about other OS-es with 64-bit JDKs (AIX, Irix?)... 

It sounds as though (not a surprise, sad to say) LiveConnect is underused --
it's clearly undertested, and it has been broken way too often in the last seven
years.

> Finally, the current definition of lcjsobject (in jsjava.h) is rather 
> imperfect. It depends on JS_BYTES_PER_LONG, but there is no set-in-stone 
> relationship between sizeof(void*) and sizeof(jlong). 

Did you file a separate bug on this, or is it covered adequately by bug 303656?

> Truly, lcjsobject should be (u)intptr_t with possible special handling for the 
> few non-C99 compilers. 

"few"?  Last someone looked (dbaron, I think), it was too many to drop support
for, so there's nothing "possible" about special handling for lack of C99 -- we
will need it.

/be

Comment 15

[Mikhail T.]

(In reply to comment #14) 
 
> SpiderMonkey's standalone tarball release does not include LiveConnect, 
AFAIK.  
 
js-1.5rc6a does contain liveconnect and perlconnect (but no xpconnect). 
Moreover, the tests suite contains sizable collections of lc* tests. 
 
I'm gearing up to enabling liveconnect in FreeBSD's spidermonkey port as soon 
as the oji-plugin is available for amd64. 
 
> It should not. 
 
I'd rather debug any possible liveconnect problems in a standalone, 
command-line executable with a nice set of tests, than inside a browser, that 
can fail for a number of other reasons. 
 
Can it (liveconnect) be added to a running js at run-time -- can js load 
modules on the fly like TCL, Perl, Python? If not, than it is, probably, better 
to compile it all in by default, because the primary use of js is to develop 
browser-extensions, is not it? 
 
> > Truly, lcjsobject should be (u)intptr_t with possible special handling for 
the  
> > few non-C99 compilers.  
>  
> "few"?  Last someone looked (dbaron, I think), it was too many to drop 
support 
> for, so there's nothing "possible" about special handling for lack of C99 -- 
we 
> will need it. 
 
Well, the special definition of ptrdiff_t appears to exist for WIN16 only. Is 
intptr_t less common than ptrdiff_t? 
 
May I suggest "a real compiler" :-)? Not just gcc, -- there is already MSVC-7, 
I heard, and Intel offers a free (for personal use) compiler back-end for MSVC 
(same core as icc on Linux and FreeBSD). 

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (In reply to comment #14) 
>  
> > SpiderMonkey's standalone tarball release does not include LiveConnect, 
> AFAIK.  
>  
> js-1.5rc6a does contain liveconnect and perlconnect (but no xpconnect). 
> Moreover, the tests suite contains sizable collections of lc* tests.

Yuck.  Those are a waste of space, and not used by the standalone SpiderMonkey
build, so I don't see why they are included.  This may date from 1998 or some
time around then, when I was working on mozilla.org and had handed JavaScript
off to a team inside Netscape.  I guess we're now stuck with shipping
liveconnect and perlconnect, though.

> I'm gearing up to enabling liveconnect in FreeBSD's spidermonkey port as soon 
> as the oji-plugin is available for amd64. 

Ok, but let us know how that goes in another forum.  This is a resolved bug,
it's not the best place for our exchange.  I'll shut up now if you want the last
word here, but cc: me elsewhere when you have news on this js+liveconnect shell.

> Can it (liveconnect) be added to a running js at run-time -- can js load 
> modules on the fly like TCL, Perl, Python? If not, than it is, probably, better 
> to compile it all in by default, because the primary use of js is to develop 
> browser-extensions, is not it?

JS (as in SpiderMonkey) can do things such as module loading only by virtue of
an embedding using the JS API to implement native functions such as Load in js.c.

Developers of browser extensions, not surprisingly, want a browser containing
the JS engine to use as a testbed -- they don't want just a JS shell, with or
without Java support (Java is kind of dead on the client, apart from a few game
and real estate virtual tour applets -- and Flash kicks the latter's ass every
time).

> Well, the special definition of ptrdiff_t appears to exist for WIN16 only. Is 
> intptr_t less common than ptrdiff_t? 

Yes.  Please question your assumption before asking questions whose answers
which I've already given contradict your assumption.

> May I suggest "a real compiler" :-)? Not just gcc, -- there is already MSVC-7, 
> I heard, and Intel offers a free (for personal use) compiler back-end for MSVC 
> (same core as icc on Linux and FreeBSD). 

Ha ha.

We ship Firefox, perhaps you've heard of it?  It runs on Windows from 98 (95 is
built by a volunteer) to XP SP2.  We have to build with several MSVC versions. 
If you live in a world where you have zero to few users, and you can dictate OS
and compiler version, then more power to you, but your world and mine are far apart.

Last we heard, Intel's compiler could not handle all of Mozilla.  That was a
while ago, I'm sure it's better now.  But who knows?  Not you.

Last comment from me, this is tiresome now, and your clue absorption rate seems
seriously low.

/be

Comment 17

[Mikhail T.]

> your clue absorption rate seems seriously low. 
 
Dear Mozilla project members!  
  
Please, put some sort of plumbing over Brendan's orifices.  
  
This **** match is escalating and -- perhaps, unlike you -- I have not seen  
much contribution (nor insight) from him to be willing to tolerate this ****. 
 
Thank you. 

Comment 18

[Johnny Stenback (:jst)]

(In reply to comment #14)
> (In reply to comment #13)
[...]
> > Truly, lcjsobject should be (u)intptr_t with possible special handling for the 
> > few non-C99 compilers. 
> 
> "few"?  Last someone looked (dbaron, I think), it was too many to drop support
> for, so there's nothing "possible" about special handling for lack of C99 -- we
> will need it.

/me starts to shiver. C99 is nowhere near usable in Mozilla yet, just look at
all the changes it took to get nptypes.h into good enough shape to merely
scratch the sruface of C99 types. It's still not good enoug, there are
outstanding problems on some systems...

  http://lxr.mozilla.org/mozilla/source/modules/plugin/base/public/nptypes.h

Comment 19

[Mikhail T.]

(In reply to comment #18)

> C99 is nowhere near usable in Mozilla yet, just look at all the changes it took 
> to get nptypes.h into good enough shape to merely scratch the surface of C99
> types.

If C99 types are considered a "good thing", then instead of inventing Mozilla's
own (jsword, JSWord, PRUint32, lcjsobject, etc.), it is better to switch to C99
type names and provide compatibility glue for the systems/compiler combinations,
that still lag behind, ONLY.

For example, the assumption that `sizeof(long)==sizeof(void*)' which is in the
current sources of spidermonkey is just as unfounded as an earlier assumption of
`sizeof(int)==sizeof(void*)'. It just happens to be true today (I think).

Three years from now, when a port to, say, Sony's PlayStation-10 is under way,
it may turn out, that longs are twice bigger than pointers (those CPUs are 128
bit already, AFAIK). But whoever makes a compiler for this hypothetical platform
is likely to offer these C99 types too. Let's just use them...

By inventing your own type names, you are pessimizing standards-compliant
systems (by making occasional breakage more likely and porting harder) *without
really helping the non-compliant ones*.

And if you think, no such "occasional breakage" may occur, well, HelixPlayer's
(a.k.a. Real) ULONG32 manages to be a 64-bit type on amd64 somehow. Had they
used the right type (uint32_t), there'd either be no problem or an immediately
obvious compile-time error, instead of a very difficult to debug run-time memory
corruption.

And, by having to maintain these special types for the compliant systems *in
addition to* the non-compliant ones, you are making your own work more complex...

Why, in other words, is jsword better than intptr_t (or uintptr_t)? Both need to
be special-handled with MSVC/Windows, but, at least, intptr_t needs no
introduction on more decent platforms...

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> If C99 types are considered a "good thing", then instead of inventing Mozilla's
> own (jsword, JSWord, PRUint32, lcjsobject, etc.), it is better to switch to C99
> type names and provide compatibility glue for the systems/compiler combinations,
> that still lag behind, ONLY.

I'm tired of you in a bad way now, so this will be the last you'll hear from me
in response to you in any bugs:

1.  SpiderMonkey predates C99, having been written by me in 1996, based on the
original "Mocha" runtime I created for Netscape 2 in 1995, so there was no
"inventing" our own types instead of reusing a standard.

2.  Mozilla is ported to many systems, too many of which lack C99 support in
full.  From two portability gurus:

[[e-mail]:]
I could see us doing that with a configure test which made a typedef for int32_t
if the environment didn't provide one, but I don't think we can make C99 a
requirement.  I think we still have tinderboxes using gcc versions from before
C99, to say nothing of the untinderboxed platforms.

Mike
(boy, though, wouldn't it be nice if we _could_ make C99 a requirement?)

[[e-mail], NSPR owner:]
It has only been four or five years since C99.  Some compilers or libc
implementations don't even support C95 features yet. 

3.  You are once again proposing a low-return, high-cost misadventure in code
that "would be nice" but that is not worth the opportunity cost to me or others
working on the project.

If *you* want to do the work, and I mean all of it, including getting the glue
right for all supported platforms that lack full and correct C99, then pull a
Mozilla trunk and get busy -- patches that actually show attention to Mozilla's
portability will get in, eventually.

But I have a better idea; get your economics right and patch something more
important.

/be

Comment 21

[Mikhail T.]

Why is the name "lcjsobject" better than "intptr_t"?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's not.  But it's a lot of work to fix it for rather little benefit.

Comment 23

[Mikhail T.]

> It's not.  But it's a lot of work to fix it for rather little benefit.

That work will have to be done eventually, and you should stop _increasing_ its
volume...

For example, jsval is a jsword (jspubtd.h), jsword is a JSWord (jscompat.h), and
JSWord is a long (unreliable in itself). Having this many aliases is just BAD
PRACTICE, and getting rid of it is a big, not little benefit. (Oh, boy, did I
just demonstrate "mean spirit"...)

This goes beyond using C99-names vs. Mozilla's own.

The, JSWord's *purpose*, according to the comment in jstypes.h is to be an
integer, that is the same size as a void*. Why was lcjsobject created a year ago
(in an incomplete fix for bug 239562)?

Moreover, SpiderMonkey is using NSPR anyway, so why does JSWord exist, instead
of simply using PRWord (nspr/prtypes.h)? Because it was created before PRWord
was? Fine, but now a tree-wide replacement can be done in one command... Hardly
"a lot of work".

If you can not just use C99 types -- fine... Switching to the base types (as
offered by NSPR) would make most of the problems I'm talking about go away. And
the (present and future) porting efforts will be much simplified, for only the
NSPR's header(s) will require adjusting...

Best of all, this -- phasing redundant types out -- can be done _gradually_. No
need to create a special branch -- there are 723 already...

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Types in the API aren't just used by our code.  They're used by other consumers
of the API.  So you can't remove them just because you've removed the uses in
our code.

Second, typedefs are useful for (a) documenting what something is and (b) when
you want to change the meaning of something.  If we were starting over in a
world with perfect C99 support, we wouldn't create JSWord, but we would create
jsval, since it has useful semantics (and, if it weren't an API that people
depend on, could potentially be changed to a different type).

Comment 25

[Mikhail T.]

(In reply to comment #24)
> Types in the API aren't just used by our code.  They're used
> by other consumers of the API.

Judging by the Firefox' tree, nothing uses neither jsword nor JSWord outside of
the js/src. Perhaps, other (non-Mozilla) products use it... Maybe. Should both
of them exist? JSWord is used for nothing else but to define jsword...

> So you can't remove them just because you've removed the uses in
> our code.

Ok, can the uses be eliminated, at least? Along with lcjsobject? And JSWord
definition can not be removed for now, can it be changed to be an alias to
PRWord for the time being?

NSPR is a standalone product with its own substantial set of tests. It can be
ported and tested independently, and we have done it...

     http://www.freshports.org/devel/nspr/

At some point we may add a local patch to make PRWord a true intptr_t, but we'd
rather not have to chase umpteen other types like that throughout Mozilla's tree.

> Second, typedefs are useful for (a) documenting what something is and (b) when
> you want to change the meaning of something. If we were starting over in a
> world with perfect C99 support, we wouldn't create JSWord, but we would create
> jsval, since it has useful semantics (and, if it weren't an API that people
> depend on, could potentially be changed to a different type).

Yes, having a jsval makes sense. But in its current form it ought to be just a
PRWord (or, in a better world, an intptr_t).

But why was lcjsobject created? Its use is not application-specific, but
platform specific -- a hardware-dependent type like PRWord should've been used
there.

In addition to prtypes.h, in js/src alone there are two *types.h headers with a
mix of type names and definitions between them. And both were visited just
recently... jsotypes.h is even trying to do, what I'm advocating -- it creates
things like "uint32". Well, the right name (per C99) is "uint32_t", but that's it.

But the first thing to do getting out of a hole, is to STOP DIGGING.

(And the second is to stop trying to bite off the head of whoever suggests, that
UP is a desirable direction -- even when he is irreverent about it.)

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

SpiderMonkey, as an embeddable Javascript engine, compiles and works fine
without NSPR. The only places where it depends on NSPR are when you define
JS_THREADSAFE and when you enable the JS file object (which is currently broken
anyway, for other reasons). Making standalone (i.e., not THREADSAFE)
SpiderMonkey depend on NSPR is not what we want to do at all. Therefore, we
cannot replace our uses of JSWord in SpiderMonkey with PRWord.

Comment 27

[Brendan Eich [:brendan]]

I've apologized to Mikhail in private in response to mail from him, but I'll do
it here too.  My comments about clue absorption were out of line, and frankly I
was generally unpleasant, for which I'm sorry too.

Again, none of the objections raised by experienced hackers here to the one-time
patch offered in this bug mean that we never want to use C99, or will use it
only if someone else does all the work.  It does mean that the trade-offs in
taking that patch and letting the ports tinderboxes burn is not right.

To work together there has to be more give and take, and better understanding of
projects' histories and differing values.  It can't all be "my way good, your
way bad".  I repeat that our position is not that harsh, even if my particular
words were harsh.  We almost certainly will use intptr_t some day.  How to make
that day come soon is not clear.

/be