Bug 4094 – UString for Windows compilation fixes

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Justin Haygood]

Fixes UString for Windows

1. Fixes proper includes
2. Allows it to use ICU

[Justin Haygood]

Created attachment 3044 [details]
Fixes UString for Windows

[Darin Adler]

Comment on attachment 3044 [details]
Fixes UString for Windows

I don't think a JavaScriptCore patch should use KWQ in the title.

As an aside: Isn't there some way to get the <stdint.h> on Windows? Maybe we
can put a file named "stdint.h" in the directory that defines the types?

Why the change from #ifdef to #if on HAVE_STRINGS_H and not on HAVE_STRING_H?

Maybe the HAVE_ICU stuff should go into config.h?

Why is the #include of <assert.h> inside an ifdef? That shouldn't be necessary.

I suggest we eliminate the use of MAX rather than defining it. There's only one
use of it in the file, and it's not standard. We could use the C++ std::max
instead.

[Justin Haygood]

(In reply to comment #2)
> (From update of attachment 3044 [details] [edit])
> I don't think a JavaScriptCore patch should use KWQ in the title.
> 

It already did.

> As an aside: Isn't there some way to get the <stdint.h> on Windows? Maybe we
> can put a file named "stdint.h" in the directory that defines the types?
> 

Good idea, but I don't know how to make a patch that uses new files.
> Why the change from #ifdef to #if on HAVE_STRINGS_H and not on HAVE_STRING_H?
> 

I'll make it match up
> Maybe the HAVE_ICU stuff should go into config.h?
> 

Why not assume that we have ICU like for function.cpp ?
> Why is the #include of <assert.h> inside an ifdef? That shouldn't be necessary.
> 

It wasn't included in the original file, but it's needed for assert() on
Windows. I'll take it out of the ifdef

> I suggest we eliminate the use of MAX rather than defining it. There's only one
> use of it in the file, and it's not standard. We could use the C++ std::max
> instead.
> 

Sounds like a better idea.

Now to get on to it.

[Justin Haygood]

Created attachment 3055 [details]
Recommended Changes

Recommended Changes Done

[Justin Haygood]

Comment on attachment 3055 [details]
Recommended Changes

Darin's recommendations are done. kjsint.h attatched separately

[Justin Haygood]

Created attachment 3056 [details]
kjsint.h

Used by Attatchment 3055

[Darin Adler]

Comment on attachment 3055 [details]
Recommended Changes

#include <xutility> is not the proper way to get std::max; the correct include
is #include <algorithm>.

The #ifdef/#if thing should not have a comment. We don't want a comment
everywhere we're using an #if explaining this general principle.

Same thing with this:

+// #include <stdint.h> replaced with kjsint.h

We don't need a record that this "used to be <stdint.h>".

Finally, I think we could just unconditionally do the ICU stuff here as we did
in that other file, although #if HAVE_ICU is also just fine with me.

Overall looks, good, but setting review- until these last few things are fixed.

[Justin Haygood]

Already done