Excellent! I'm really looking forward to those patches. I have one request: please make sure that you are building the new version using SpiderMonkey 31 which I merged to master this past Friday.

For example,

> Use jschar instead of uint16_t for the parts where we convert to UTF-16, since
> SpiderMonkey uses wchar_t for jschar, not uint16_t, on Windows.

...this problem will go away since SpiderMonkey should use char16_t across the board now.

(In reply to Fan, Chun-wei from comment #0)
> I will try to attach patches in a few days, after travels--please remind me
> if I forget in a week or two.

This is the reminder you asked for :-)

Hi Philip,

Sorry for holding up on the issue, unfortunately I we not able to do a SpiderMonkey31-based build that does not crash on startup.

So, I think to work on it again based on the latest commits to see whether things are in better shape, or I need to start building up from 1.47.0, which was the point that worked, albeit with SpiderMonkey24, and find out what it really going on.

With blessings, thank you!

No problem. If you want to save yourself some work, I am planning to continue upgrading SpiderMonkey as soon as possible and I expect to have GJS based on SpiderMonkey 38 by the time of the GNOME 3.24 code freeze in February. So if you are spending time on getting a working SpiderMonkey build on Windows, then I might suggest continuing directly with 38.

If you post the patches that you had for 1.47.0, I would still be happy to review them. I'm sure some of them will still apply, and we can get those in early.

If you need advice on getting SpiderMonkey to work on Windows, I have now gotten myself somewhat experienced in getting it to build on Linux and macOS. If you have any specific questions, I might be able to help. I can recommend configuring it with --enable-debug, in any case, since that provides much better error messages when SpiderMonkey crashes.

Does SpiderMonkey's own JS REPL also crash on startup, or is it just GJS?

Created attachment 343529 [details] [review]
Windows: Don't include unistd.h and sys/mman.h unconditionally

Hi Philip,

This is what I have based on the latest 1.47.4.  I think I am starting here as it seems that the crash according to the Visual Studio debugger is caused when the following is called in context.cpp on startup:

if (!JS::Evaluate(cx, global, options, lie_code, lie_length, &promise)) {
        g_bytes_unref(lie_bytes);
        return false;
    }

Due to a stack buffer overflow.  It does seem to me that lie_code and lie_length are successfully acquired.  This seems to be the case since I started working on the mozjs-31 based builds

Also, js.exe (if I understood you correctly) started normally.

Anyways, here comes the patchset...

With blessings, thank you!

Created attachment 343530 [details] [review]
Headers: Avoid GCCisms

Hi,

Some items in the headers have GCCisms, which must be dealt with to build on non-GCC/non-g++.

With blessings, thank you!

Created attachment 343531 [details] [review]
Avoid hardcoding paths and g_autoptr

Hi,

This turns the code back into the traditional allocate-use-free approach as g_autofree is only supported on GCC and perhaps CLang.  Since SpiderMonkey is normally supported on Windows using Visual Studio, this is necessary (one cannot use mix Visual Studio builds with MinGW-built items).

Also, avoid hardcoding paths at build time, but construct the paths dynamically at run time on Windows--this is what is done on the reste of the GTK+ stack.

With blessings, thank you!

Created attachment 343532 [details] [review]
Public Headers: Use symbol export macros

Hi,

This decorates the public symbols in the headers so that they can be exported during the build and imported when used.

With blessings, thank you!

Review of attachment 343529 [details] [review]:

::: gi/function.cpp
@@ +42,3 @@
 #include <girepository.h>
+
+#ifndef G_OS_WIN32

It compiles just fine for me if you simply remove these two headers, I guess they weren't needed!

Review of attachment 343530 [details] [review]:

::: util/log.h
@@ +112,3 @@
 #endif
 
+#ifdef G_HAVE_ISO_VARARGS

I would be OK with just switching to the ISO style unconditionally here.

@@ +115,3 @@
+
+#if GJS_VERBOSE_ENABLE_PROPS
+#define gjs_debug_jsprop(topic,...) \

Coding style: space after commas please.

Review of attachment 343531 [details] [review]:

Aha, I was discussing with Cosimo a few weeks ago about whether to allow g_autofree in the GJS codebase. We couldn't see any reason not to, but we overlooked this reason.

How are you adapting GLib and other packages which are starting to use g_autofree?

Anyway, this is C++, so we should be able to get g_autofree functionality with std::unique_ptr. Instead of converting back to "goto out" and freeing at the end, let's use that. I've been trying to think of some good syntactic sugar for that, but it hasn't been a high priority. I'll move it higher up my to-do list now.

Could you split out the context.cpp change into its own patch please? That part looks fine to commit already.

Review of attachment 343532 [details] [review]:

::: Makefile.am
@@ +39,3 @@
 	gjs/coverage.h			\
 	gjs/gjs.h			\
+	gjs/gjs-macros.h		\

Can we call it macros.h for consistency with the other header files?

::: gjs/gjs-macros.h
@@ +1,3 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2008  litl, LLC

Replace this with 2017 and your own copyright.

@@ +24,3 @@
+#include <glib.h>
+ 
+#ifndef GJS_EXP

Better to use the usual #ifndef GJS_MACROS_H around the whole file, instead of #ifndef around the individual macro.

Can we call it GJS_EXPORT? It's a bit more typing but reads clearer in my opinion.

::: util/error.h
@@ +25,3 @@
 #define __GJS_UTIL_ERROR_H__
 
+#include <gjs/gjs-macros.h>

I prefer to keep #include <glib.h> here explicitly, even though gjs-macros.h brings it in, because it's required for the code below.

Thank you so much for the patches! I mostly had just some minor comments on each one, but I think we can nicely use C++ language features to replace the g_autoptr instead of going back to C-style.

(In reply to Fan, Chun-wei from comment #5)
> This is what I have based on the latest 1.47.4.  I think I am starting here
> as it seems that the crash according to the Visual Studio debugger is caused
> when the following is called in context.cpp on startup:
> 
> if (!JS::Evaluate(cx, global, options, lie_code, lie_length, &promise)) {
>         g_bytes_unref(lie_bytes);
>         return false;
>     }
> 
> Due to a stack buffer overflow.  It does seem to me that lie_code and
> lie_length are successfully acquired.  This seems to be the case since I
> started working on the mozjs-31 based builds

What happens if you comment out that JS::Evaluate call and the rest of the function beyond it? Does GJS function normally in that case?

Hi Philip,

Thanks for the feedback for the patches, I will get to them tomorrow here.

About how things went on the crashing issue:

I actually went a bit further to work up from 1.47.0, which was based on SpiderMonkey24, to the point when we transitioned into SpiderMonkey31, up to 1.47.3, which was before the point when we incorporated _lie.js in the resources...

The gjs builds on Windows did work up to commit 46e22cf (build: Include docs in tarball), and the example scripts that I had the .typelibs for ran fine (i.e. everything besides clutter.js).

As we switched to SpiderMonkey31 after commit bce2da5 (build: Build with mozjs31), the C++ source build would only succeed when we reached cd355ea (js: Fix misc APIs for mozjs31), and from that point any build of the gjs executable will crash, even after incorporating the JS_Init() call via DllMain() or the constructor (on Windows, for DLLs, DllMain() gets executed first--see in GLib, GTK+ on how it is done).  As I am not sure whether the transition from SpiderMonkey24 was done at commit cd355ea plus the commits 3739c71 (js: Don't exit immediately from System.exit()), commit 2eb640d (object: Enter global compartment in custom constructor) and calling JS_Init() in the constructor/DllMain(), I decided to try out 1.47.3, which actually crashed at gjs_context_eval() during startup when calling gjs_eval_with_scope(), which in turn calls

if (!JS::Evaluate(context, eval_obj, options, script, script_len, retval))
        return false;

which actually crashes with an access violation (aka segfault), and this is also the case when I comment out the part where the _lie.js/promise script is loaded.

Somehow, since on Windows jschar is handled differently (please see initial bug report) (and hence char16_t) for SpiderMonkey31 and likely 38, this *might* be something I would probably want to look into further, as char16_t is taken as wchar_t on Windows/MSVC in SpiderMonkey31.

With blessings, thank you!

The transition to SpiderMonkey 31 was started with commit bce2da5 (build: Build with mozjs31) but was only complete with commit 72c0298 (js: Workaround for function with custom prototype). Anything in between might compile after a certain point, but will not work correctly.

Even if you comment out the whole gjs_define_promise_object() call in lines 516-517 of context.cpp, does the crash still happen?

What might help is to build SpiderMonkey with --enable-debug. This compiles in a lot more assertions so that misuses of the SpiderMonkey API fail earlier and with clearer error messages. (Though I'm running exclusively with --enable-debug, so it would have to be a Windows-only problem...)

Thanks for working on this!

Created attachment 343765 [details] [review]
Windows: Don't include unistd.h and sys/mman.h unconditionally (take ii)

Hi Philip,

This is the updated patch for the inclusion of unistd.h and sys/mman.h, as suggested...

Created attachment 343766 [details] [review]
Headers: Avoid GCCisms (take ii)

Hi,

This is the removal of the GCCisms in the headers by using only ISO-style varargs syntax and G_GNUC_UNSED...

Created attachment 343767 [details] [review]
Windows: Use DllMain()

Hi Philip,

For the build-time hardcoded paths issue, I think I decided to use a DllMain() instead, as we could get a HMODULE for free when we make use of it, which can be fed to g_win32_get_package_installation_directory_of_module(), so that we can construct the paths to acquire the items for PKGLIBDIR and GJS_JS_DIR at run-time, which will make libgjs relocatable, which is often the case on Windows (and is done in GLib/GTK+ and so on).

The other reason why this approach is used is that we can reduce the use of compiler constructors on Windows so that we get assured that initialization and shutdown routines (such as JS_Init() and JS_ShutDown()) can be called when the DLL is loaded or is exited.  There might be other things AFAICT that are done through constructors, and probably they might be better done through DllMain() in Windows--perhaps there are some other things that can be done here on Windows for the initialization part, as AFAICT compiler constructors are used quite a bit in libgjs.

(In reply to Philip Chimento from comment #15)
> 
> Even if you comment out the whole gjs_define_promise_object() call in lines
> 516-517 of context.cpp, does the crash still happen?

There is still another crash (access violation/segfault), when I tried to do so, which is identical to the crash I had for 1.47.3 (which was before the inclusion of the promise object).  When I started gjs.exe, what I saw happened was console.cpp calling gjs_context_eval() (in main(), around line 281), which calls gjs_eval_with_scope() (around line 738 in context.cpp), which then calls JS::Evaluate() (in jsapi-util.cpp around line 882), which then crashes in Evaluate() when calling setCompileAndGo() (in SpiderMonkey31's jsapi.cpp).  My suspicions are that the eval_obj was not properly set up, for it does look like obj was not a valid pointer (or even NULL).  This means that it crashed during gjs.exe's initialization stage.  I will try to attach a call stack ASAP.
 
> What might help is to build SpiderMonkey with --enable-debug. This compiles
> in a lot more assertions so that misuses of the SpiderMonkey API fail
> earlier and with clearer error messages. (Though I'm running exclusively
> with --enable-debug, so it would have to be a Windows-only problem...)

No joy with this :(

With blessings, thank you!

Created attachment 343768 [details]
CallStack

Hi,

This is the call stack when the promise object items are commented out.  The top image is the call stack and the bottom image is the variable state.  Sorry for the non-English items in there--the 第xxx行 means line xxx.

With blessings, thank you!

Created attachment 343769 [details] [review]
Public Headers: Use symbol export macros (take ii)

Hi,

This is the updated patch for symbol export with the updated file names and macro names.

I currently left out the part where we get rid of g_autofree, as using std:unique_ptr most probably requires a patch of its own.

For GLib and the GTK+ stack, the consensus we have is to not use the g_autofree and related items if the code is going to be compiled by compilers other than GCC, so this is actually not dealt with in GLib, GTK+ etc. as they are not supposed to be used in these libraries.

With blessings, thank you!

Review of attachment 343766 [details] [review]:

::: util/log.h
@@ +154,3 @@
 #endif
 
+

Coding style: remove extra line here, but I'll just remove that before I commit it.

Review of attachment 343767 [details] [review]:

::: gjs/runtime.cpp
@@ +253,3 @@
+
+  case DLL_THREAD_DETACH:
+{

Does this mean that GPrivate is not destructed automatically when the thread exits on Windows?

@@ +298,2 @@
     if (!runtime) {
+#ifndef G_OS_WIN32

Instead of taking this out, how about adding

    static bool gjs_is_inited = false;

to the above block, and setting gjs_is_inited = true in the DLL_PROCESS_ATTACH case? Then we can still keep this assertion.

Review of attachment 343769 [details] [review]:

Just some fairly trivial comments. If you don't have time to fix them up, let me know and I'll do it before committing.

::: gjs/macros.h
@@ +24,3 @@
+#include <glib.h>
+ 
+#ifndef GJS_EXP

Should be GJS_EXPORT. But instead, it would be better to use regular #ifndef GJS_MACROS_H guards around the whole file, rather than #ifndef around the macro.

@@ +25,3 @@
+ 
+#ifndef GJS_EXP
+# if defined (G_OS_WIN32) 

#if defined -> #ifdef ?

Review of attachment 343765 [details] [review]:

+1

Review of attachment 343769 [details] [review]:

::: gjs/macros.h
@@ +1,3 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2008  litl, LLC

Actually I need your copyright information here before I can commit it :-)

As for the crash, based on a similar thing that was happening on Fedora's mozjs31, it seems like you might be suffering from https://bugzilla.mozilla.org/show_bug.cgi?id=1083464. Try compiling SpiderMonkey 31 with these two patches applied:

- https://hg.mozilla.org/releases/mozilla-esr31/rev/15d1c2a4ea55
- https://hg.mozilla.org/releases/mozilla-esr31/rev/d74ec3052a66

To check if this is what's happening, you might examine the stack before and after executing the statement "JS::CompileOptions options;" If you're suffering from this bug, then that statement will corrupt the program's stack.

Or make a quick check by adding "#define JSGC_USE_EXACT_ROOTING 1" to gjs/jsapi-wrapper.h, that worked around the Mozilla bug in the Fedora case.

Created attachment 344105 [details] [review]
Windows: Use DllMain() (take ii)

Hi Philip,

Sorry for the delay...

(In reply to Philip Chimento from comment #22)
> 
> Does this mean that GPrivate is not destructed automatically when the thread
> exits on Windows?

I think so-a similar thing is done in GLib on Windows as well, which prevents a leak of GPrivate items.

> 
> @@ +298,2 @@
>      if (!runtime) {
> +#ifndef G_OS_WIN32
> 
> Instead of taking this out, how about adding
> 
>     static bool gjs_is_inited = false;
> 
> to the above block, and setting gjs_is_inited = true in the
> DLL_PROCESS_ATTACH case? Then we can still keep this assertion.

Hmm, we do check whether JS_Init() was successful by doing it in an g_assert()..., but this is added anyways.

(In reply to Philip Chimento from comment #26)
> As for the crash, based on a similar thing that was happening on Fedora's
> mozjs31, it seems like you might be suffering from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1083464...

(In reply to Philip Chimento from comment #27)
> Or make a quick check by adding "#define JSGC_USE_EXACT_ROOTING 1" to
> gjs/jsapi-wrapper.h, that worked around the Mozilla bug in the Fedora case.

Yup, I got bitten by this :|.  Adding the #define worked to fix the issue (and the Promise module loaded successfully :) ).

With blessings, thank you!

Created attachment 344125 [details] [review]
Don't use g_autofree

Hi,

It seems that the linked bug report that this bug depends on might require quite a bit of changes for std::unique_ptr (and the patch there probably does not cover the things needed here), so for the moment, use the traditional allocate-and-free approach.

This uses g_free() for GLib-allocated items and js_free() for SpiderMonkey-allocated items.

I won't be too sad if this particular patch doesn't make it in :).

With blessings, thank you!

Created attachment 344127 [details] [review]
Public Headers: Use symbol export macros (take iii)

Hi,

Updated patch as suggested, with copyright item added.

With blessings, thank you!

Created attachment 344129 [details] [review]
modules/system.cpp: Include unistd.h on non-Windows only

Hi,

I didn't get to check on non-Windows to see whether unistd.h is needed there, so this avoids including unistd.h in modules/system.cpp on Windows, as its functionality seems to be not used there.

With blessings, thank you!

Review of attachment 344129 [details] [review]:

+1

::: modules/system.cpp
@@ +30,3 @@
 #include <gjs/context.h>
 
+#ifndef G_OS_WIN32

It seems to compile just fine without unistd.h at all, so I'll just remove this and commit it.

Review of attachment 344127 [details] [review]:

+1

::: gjs/macros.h
@@ +1,3 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2008  litl, LLC

I think we can remove litl from this file, but I'll just fix that up before I commit it.

Review of attachment 344105 [details] [review]:

+1

Review of attachment 344125 [details] [review]:

Feel free to commit this one on a side branch if you want a temporary place to build from, but I would rather find a good C++ solution. I expect to be able to tackle that soon and it won't take too long.

Created attachment 344680 [details] [review]
libgjs-private/: Export _get_type() symbols

Hi,

This exports the _get_type()'s as they are needed for building GjsPrivate-1.0.gir, as the linker will look for them.

With blessings, thank you!

Created attachment 344681 [details] [review]
build: Separate source listings from Makefile.am and Makefile-modules.am

Hi,

This separates the source listings into gjs-srcs.mak (for libgjs and gjs-console) and gjs-modules-srcs.mak (for the GJS modules) so that they can be shared with different Makefile-based build systems.  This will allow addition of NMake Makefiles that can be used to build GJS while reducing repetition, which I am now readying.

With blessings, thank you!

Created attachment 344682 [details]
Minimal Test Case

Hi,

In the course of supporting GJS on Windows, it seems that there is a problem on x64 when I was testing the build out with gjs-examples[1], where running most, if not all, of the examples there crash on 64-bit (x64) but runs fine on 32-bit (x86).

It turns out that the crash occured at arg.cpp in gjs_value_from_g_argument() during the call:

if (G_TYPE_IS_INSTANTIATABLE(gtype) ||
                G_TYPE_IS_INTERFACE(gtype))
                gtype = G_TYPE_FROM_INSTANCE(arg->v_pointer);

Where the crash occurred at gtype = G_TYPE_FROM_INSTANCE(arg->v_pointer), when trying to do Gio.File.new_for_path() in the JS file.  The attached test.js is the minimal test case that I can come up with that causes the crash--I am suspecting about the overrides (which I have yet been able to dig further into, as that function is in the overrides, IMHO), and I think this is most probably due to Windows having sizeof(void*|gpointer) is 8, not 4 like on 32-bit Windows.

[1]: https://github.com/optimisme/gjs-examples

With blessings, thank you!

Review of attachment 344680 [details] [review]:

+1

Review of attachment 344681 [details] [review]:

I think you attached the wrong patch here? It's the same as the other one.

Review of attachment 344680 [details] [review]:

Hi Philip,

Thanks, I pushed the patch as 980b9cb.

With blessings, thank you!

Created attachment 344828 [details] [review]
build: Separate source listings from Makefile.am and Makefile-modules.am

Hi Philip,

(In reply to Philip Chimento from comment #40)
> I think you attached the wrong patch here? It's the same as the other one.

Indeed I did, sorry!

Created attachment 344829 [details] [review]
Visual Studio builds: Add a pre-configured config.h(.win32.in) template

Hi,

This adds a pre-configured config.h(.win32.in) that is suitable for use for Visual Studio builds--this assumes Visual Studio 2013 or later as the code base requires C++-11 support that is available in Visual Studio 2013 or later.  This is also done as a template so that we can ensure it has the correct version info during a release.

With blessings, thank you!

Review of attachment 344828 [details] [review]:

Sounds like a good plan, just a few comments.

::: Makefile-modules.am
@@ +24,3 @@
 EXTRA_DIST += $(modules_resource_files) $(srcdir)/modules/modules.gresource.xml
 
+include gjs-modules-srcs.mak

I think .mk is the more usual extension.

::: gjs-srcs.mak
@@ +4,3 @@
+	gjs/gjs.h		\
+	gjs/macros.h		\
+	util/error.h

Using "$(NULL)" on the last line is preferred now, since you're refactoring this anyway

@@ +6,3 @@
+	util/error.h
+
+gjs_former_public_headers =	\

As long as you're refactoring this, I would take this opportunity to just squash gjs_former_public_headers, gjs_srcs, gjs_gi_srcs, and gjs_private_srcs all together in one variable. You may have to rewrite the comments in Makefile.am a bit to get them to make sense. Also, let's take this opportunity to alphabetize them (makes it easier to see if one is missing.)

Can you explain a bit more about the pre-configure process? Is config.h.win32.in actually hand-written, to match the configuration of VS2013? Or is it generated somehow?

Created attachment 344878 [details] [review]
build: Separate source listings from Makefile.am and Makefile-modules.am ()

Hi Phhilip,

Here comes the new patch for splitting the sources from Makefile.am and Makefile-modules.am...

(In reply to Philip Chimento from comment #44)
> Review of attachment 344828 [details] [review] [review]:
> > As long as you're refactoring this, I would take this opportunity to just
> squash gjs_former_public_headers, gjs_srcs, gjs_gi_srcs, and
> gjs_private_srcs all together in one variable. You may have to rewrite the
> comments in Makefile.am a bit to get them to make sense. Also, let's take
> this opportunity to alphabetize them (makes it easier to see if one is
> missing.)

I incorporated the suggestions; however, I kept the gjs_private_srcs on its own as that is being used for introspection for GjsPrivate-1.0.[gir|typelilb], as I saw the original build rule in Makefile.am specified the private sources, not the whole set of sources.

With blessings, thank you!

Created attachment 344879 [details] [review]
Visual Studio builds: Add a pre-configured config.h(.win32.in) template (take ii)

Hi Philip,

It seems that this patch needed a bit more updates as we need to get the integer version into win32/config.h.win32.in... by using AC_SUBST in configure.ac

(In reply to Philip Chimento from comment #45)
> Can you explain a bit more about the pre-configure process? Is
> config.h.win32.in actually hand-written, to match the configuration of
> VS2013? Or is it generated somehow?

I think to put it more accurately this is how it is done:
-The compiler checks requested in config.h.win32.in are hand-written,
 based on the entries in config.h.in, for Visual Studio 2013 and later,
 and having MinGW in mind (though I didn't test the MinGW part).  The
 autotools stuff won't support Visual Studio builds unless it is patched
 extensively, or integrates what gnulib does, which is not trivial.

-The version and package info stuff are filled in during configure.

With blessings, thank you!

Created attachment 344880 [details] [review]
NMake Makefiles for building GJS

Hi,

This is the set of NMake Makefiles that can be used by Visual Studio 2013 or later to build GJS, with a README.txt to explain the process.  Obviously this will need to get rid of g_autofree and g_autoptr() for the NMake Makefiles to work (and it seems that their appearances are on the rise lately :| ).

This is modularized, so that some items can be shared across projects (such as HarfBuzz)--I do understand some bits are still in *.mak--let me know if changing to *.mk is really desired.

With blessings, thank you!

Review of attachment 344878 [details] [review]:

+1, feel free to commit with this minor revision.

::: gjs-srcs.mk
@@ +8,3 @@
+
+# For historical reasons, some files live in gi/
+# The following headers were formerly public

I think it's not necessary to list the whole thing in detail, if people want to know this they can consult the git history. I'd change this to "Also, some headers were formerly public for external modules" and delete the list.

Review of attachment 344879 [details] [review]:

I had a couple of questions, but really, you're the expert here :-) Feel free to commit it.

::: win32/config.h.win32.in
@@ +32,3 @@
+
+/* Define to 1 if printf() accepts '%Id' for alternative integer output */
+#define HAVE_PRINTF_ALTERNATIVE_INT 1

I seem to remember "%I" meant something else on Windows.

@@ +63,3 @@
+
+/* Define to the sub-directory where libtool stores uninstalled libraries. */
+#define LT_OBJDIR ".libs/"

Is the VS build really using libtool like this?

Review of attachment 344880 [details] [review]:

One proofreading correction in a few places: interpretor -> interpreter.

Other than that, if it works, by all means commit it. But... keep in mind that this will need to be maintained and I don't have a Windows machine nor the expertise to do it. Are you volunteering? :-)

Review of attachment 344878 [details] [review]:

Hi Philip,

I pushed the patch with the suggested change as becac60, with the suggested change.

With blessings, thank you!

Review of attachment 344879 [details] [review]:

Hi Philip,

I pushed the patch as a94c06c.

With blessings, thank you!

::: win32/config.h.win32.in
@@ +32,3 @@
+
+/* Define to 1 if printf() accepts '%Id' for alternative integer output */
+#define HAVE_PRINTF_ALTERNATIVE_INT 1

As far as I can tell, activating %Id itself not cause problems for Visual Studio builds (the test snippet will pass without related warnings, and no warning was produced when the x64 compatibility check (/Wp64) flag was used, which means it is safe to enable it here.  The crash I mentioned for 64-bit builds occur with or without this #define enabled.

@@ +63,3 @@
+
+/* Define to the sub-directory where libtool stores uninstalled libraries. */
+#define LT_OBJDIR ".libs/"

The Visual Studio builds do not use libtool itself, this was written this way to be consistent across the board (this is done in GLib, GTK+ etc, as the original config.h.win32.in there were generated via custom configure calls).

Review of attachment 344880 [details] [review]:

Hi Philip,

Thanks for the spelling correction!  I pushed this with the corrected spelling as 23b72e9.

I am willing to maintain these, especially the official supported way of building SpiderMonkey is via Visual Studio on Windows, and we can't just try to use MinGW/mingw-w64 with this since this is C++.

What's left to tackle is to get rid of the g_autofree and g_autoptr() with the C++ equivalents and to nail down the x64 problem on Gio.File.new_for_path(), and gjs would be ready for Windows.

With blessings, thank you!

(In reply to Fan, Chun-wei from comment #54)
> What's left to tackle is to get rid of the g_autofree and g_autoptr() with
> the C++ equivalents and to nail down the x64 problem on
> Gio.File.new_for_path(), and gjs would be ready for Windows.

Great! Thank you for all this hard work.

I am not sure what the cause of the x64 problem might be. Can you examine the value of arg->v_pointer in that case? arg is a GIArgument which is a union of a bunch of different types, but a pointer should definitely fit in there.

I noticed in your build documentation that you mentioned the files showing up as mozjs-. The same thing happened to me, I had to patch the mozjs38 sources in jhbuild [1].

[1] https://git.gnome.org/browse/jhbuild/tree/patches/mozjs38-release-number.patch

Hi Philip,

Thanks very much, as it seems better to have the .DLL/.lib with the proper naming.
I mentioned about your patch in the Visual Studio build documentation, where people can apply (the SpiderMonkey sources are also built on Visual Studio through a special version of autotools where mozilla.org provides in the mozilla-build package, so Visual Studio builds will benefit from your patch as well).

With blessings, thank you!

Created attachment 346127 [details] [review]
SpiderMonkey 38: Fix DllMain()

Hi,

It appears that we need to call JS_ShutDown() during DLL_THREAD_DETACH after gjs_destroy_runtime_for_current_thread(), so that programs using libgjs on Windows will terminate properly, otherwise they will not terminate as the SpiderMonkey engine is not yet shut down (please see https://bugzilla.gnome.org/show_bug.cgi?id=777597#c20).  This makes it more in-line with what is done on other platforms in the destructor used as well.

Also, simplify DLL_PROCESS_ATTACH, as we could just use the results of JS_Init(), and assert that had succeeded, since it already returns a bool.  Note that we can't do this in DLL_THREAD_ATTACH as we check for the JS_Init() results before we get into DLL_THREAD_ATTACH.

With blessings, thank you!

Review of attachment 346127 [details] [review]:

+1

I can't test this, so if it works for you, excellent :-)

That last comment sounded more flip than I meant it to. I should clarify, I certainly agree with your reasoning for the change as well.

Created attachment 347488 [details] [review]
gi/args.cpp: Fix is_safe_to_store_in_double() when T=guint64

Hi,

As std::abs() is really meant for signed numbers, and Visual Studio is quite strict on types, we need a specialized implementation of is_safe_to_store_in_double() for T==guint64, otherwise the build will fail with C2668: ambiguous call to overloaded function 'abs' as there (understandably) isn't one defined for unsigned __int64 (which is what guint64 is on Windows).

This patch adds such an implementation of is_safe_to_store_in_double() for T==guint64.

With blessings, thank you!

Review of attachment 347488 [details] [review]:

Thanks for catching this! We only instantiate is_safe_to_store_in_double() for int64_t and uint64_t anyway, so maybe it's best to move to just having two overloaded functions, if the code can't be identical.

Nitpick: prefer int64_t and uint64_t to gint64 and guint64.

I went ahead with this approach in bug 779399.

Review of attachment 344125 [details] [review]:

This patch is superseded by bug 777597, so I'll mark this patch as rejected. I think nothing else needs to be done for this bug, so I'll close it.