WP8/WinRT support preview (take two)

[alexey-pelykh]

No description provided.

[dicej]

Thanks, Alexey. This looks really good. I have a few comments:

• A couple of files reference avian-interop.h, but I don't see that file in the source tree.
• It looks like you uncommented the printTrace call in throw_ for debugging; we should probably comment that out again
• There are a lot of preprocessor conditionals. Many of them are necessary, but in some cases, I think we could eliminate them. One example is the fprintf/OutputDebugString code in machine.cpp; I think it would be better to define a new function (e.g. logDebug(const char* format, ...)) that abstracts away the platform differences.
• I noticed you replaced the dynamic array allocation (e.g. "int32_t counts[dimensions]" changed to "int32_t* counts = new int32_t[dimensions]" in many places, since the MS compiler doesn't support stack-based dynamically-sized arrays. We have a macro called RUNTIME_ARRAY (defined in common.h) that I prefer to use for these cases, since it allows us to still use stack allocation with compilers that support it, and the array is automatically allocated and freed when using the MS compiler.

I'd like to address these issues before I merge the changes. If you'd like to do that, let me know. Otherwise, I'll work on it when I get a chance.

[alexey-pelykh]

• avian-interop comes from https://github.com/osmandapp/avian-wp8
• done
• this is extension, not replacement. Thus it will be just function copy-paste without ifdefs. OutputDebugString is not replacement of fprintf in that situation, at least I did not intend that at all.

[dicej]

On Mon, 4 Feb 2013, Alexey Pelykh wrote:

• this is extension, not replacement. Thus it will be just function
  copy-paste without ifdefs. OutputDebugString is not replacement of
  fprintf in that situation, at least I did not intend that at all.

Ah, yes, I misread it. Still, I feel like it could be abstracted away
into a function that writes using just fprintf on non-Windows platforms
and both fprintf and OutputDebugString on Windows. That way, we won't
have to keep the two calls in sync.

e.g.

void
logDebug(const char* format, ...)
{
va_list a;
va_start(a, format);
const unsigned length = 256;
char buffer[length];
vsnprintf(buffer, length, format, a);
buffer[length - 1] = 0;
va_end(a);

fprintf(stderr, "%s", buffer);
#ifdef PLATFORM_WINDOWS
OutputDebugStringA(buffer);
#endif
}

[alexey-pelykh]

It can be done, but then that method will be completely rewritten, since you don't have a single format string

[dicej]

On Mon, 4 Feb 2013, Alexey Pelykh wrote:

It can be done, but then that method will be completely rewritten, since you
don't have a single format string

I don't understand what you mean by that. I'm attaching a patch to show
what printTrace would look like. We'll see if GitHub's issue tracker
understands attachments.

Note that logDebug would protably need to take a FILE* too, so we can pass
errorLog(t) to it.

diff --git a/src/machine.cpp b/src/machine.cpp
index dd81197..dc2ab47 100644
--- a/src/machine.cpp
+++ b/src/machine.cpp
@@ -4752,34 +4752,19 @@ printTrace(Thread* t, object exception)

   for (object e = exception; e; e = throwableCause(t, e)) {
     if (e != exception) {
-      fprintf(errorLog(t), "caused by: ");
-#if defined(PLATFORM_WINDOWS)
-      OutputDebugStringA("caused by: ");
-#endif
+      logDebug("caused by: ");
     }

-    fprintf(errorLog(t), "%s", &byteArrayBody
+    logDebug("%s", &byteArrayBody
             (t, className(t, objectClass(t, e)), 0));
-#if defined(PLATFORM_WINDOWS)
-    OutputDebugStringA((const CHAR*)&byteArrayBody
-            (t, className(t, objectClass(t, e)), 0));
-#endif

     if (throwableMessage(t, e)) {
       object m = throwableMessage(t, e);
       THREAD_RUNTIME_ARRAY(t, char, message, stringLength(t, m) + 1);
       stringChars(t, m, RUNTIME_ARRAY_BODY(message));
-      fprintf(errorLog(t), ": %s\n", RUNTIME_ARRAY_BODY(message));
-#if defined(PLATFORM_WINDOWS)
-      OutputDebugStringA(": ");
-      OutputDebugStringA(RUNTIME_ARRAY_BODY(message));
-      OutputDebugStringA("\n");
-#endif
+      logDebug(": %s\n", RUNTIME_ARRAY_BODY(message));
     } else {
-      fprintf(errorLog(t), "\n");
-#if defined(PLATFORM_WINDOWS)
-      OutputDebugStringA("\n");
-#endif
+      logDebug("\n");
     }

     object trace = throwableTrace(t, e);
@@ -4793,36 +4778,18 @@ printTrace(Thread* t, object exception)
         int line = t->m->processor->lineNumber
           (t, traceElementMethod(t, e), traceElementIp(t, e));

-        fprintf(errorLog(t), "  at %s.%s ", class_, method);
-#if defined(PLATFORM_WINDOWS)
-        OutputDebugStringA("  at ");
-        OutputDebugStringA((const CHAR*)class_);
-        OutputDebugStringA(".");
-        OutputDebugStringA((const CHAR*)method);
-        OutputDebugStringA(" ");
-#endif
+        logDebug("  at %s.%s ", class_, method);

         switch (line) {
         case NativeLine:
-          fprintf(errorLog(t), "(native)\n");
-#if defined(PLATFORM_WINDOWS)
-          OutputDebugStringA("(native)\n");
-#endif
+          logDebug("(native)\n");
           break;
         case UnknownLine:
-          fprintf(errorLog(t), "(unknown line)\n");
-#if defined(PLATFORM_WINDOWS)
-          OutputDebugStringA("(unknown line)\n");
-#endif
+          logDebug("(unknown line)\n");
+
           break;
         default:
-          fprintf(errorLog(t), "(line %d)\n", line);
-#if defined(PLATFORM_WINDOWS)
-          OutputDebugStringA("(line ");
-         char buf[35];
-         OutputDebugStringA(itoa(line, buf, 10));
-          OutputDebugStringA(")\n");
-#endif
+          logDebug("(line %d)\n", line);
         }
       }
     }

[alexey-pelykh]

Ah, got it. Ok, I'll update that soon

[alexey-pelykh]

It would be awesome if you could advise something on previous issue regarding performance.

[alexey-pelykh]

That should be good now, also rebased

[dicej]

Thanks, Alexey. Can this branch actually be compiled for WinRT/WP8 by itself, or is additional code needed (e.g. avian-interop.h)?

[alexey-pelykh]

Same relation as your "win32" repo has.

[dicej]

Ah, yes, sorry -- you answered that earlier. I'll do some more testing and then push it if it looks good.

[dicej]

Hi Alexey. I've done some testing and fixed a couple of things: https://github.com/dicej/avian/commits/master. One thing I found but wasn't sure how to fix is that Java_java_io_RandomAccessFile_open doesn't work anymore on desktop Windows, since we're casting a wchar_t* to a const char*, which always results in "No such file or directory" when we call ::open. Do you have any thoughts on how to fix that?

[alexey-pelykh]

I'll fix that today. Actually it should be _wopen. Maybe it will also work for WP8 to avoid native calls usage. Or, as an option, I can write Win32 calls for Windows instead of ::open

[alexey-pelykh]

I've added small change to makefile