[webkit-reviews] review denied: [Bug 14206] MSVC7 support and PLATFORM define fixes for Windows : [Attachment 16561] New patch with changes made

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 14206: MSVC7 support and PLATFORM define fixes for Windows
http://bugs.webkit.org/show_bug.cgi?id=14206

Attachment 16561: New patch with changes made
http://bugs.webkit.org/attachment.cgi?id=16561&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
 #include <stdint.h>
 #include <stdlib.h>
+#include <stdarg.h>

These #include statements should be sorted alphabetically.

+#if COMPILER(MSVC7)
+// MSVC8 and above define this function
+inline int vsnprintf(char *str, size_t size, const char* format, ...) 
+{
+    int result;
+    va_list args;
+    va_start(args, format);
+    result = _vsnprintf(str, size, format, args);
+    va_end(args);
+    return result;
+}
+#endif
+

I don't think it's a good idea to define this function if it's line-for-line
identical to the snprintf function defined just above it. Can we just do
#define vsnprintf snprintf?

-#if PLATFORM(WIN)
+#if PLATFORM(WIN_OS)
 #ifndef WINVER

-#elif PLATFORM(WIN)
+#elif PLATFORM(WIN_OS)
     if (IsDebuggerPresent()) {

I'm not sure this is the right check to make. For instance, I don't think
Qt/Win wants to be calling IsDebuggerPresent (or even links against the
necessary library to obtain this symbol). Perhaps this should be
COMPILER(MSVC)? I'm not completely sure though.

 #include <kjs/identifier.h>
 #include <wtf/Vector.h>
+#include <wtf/StringExtras.h>

Please keep the #includes sorted.

 #include <wtf/Platform.h>
 #include <wtf/Vector.h>
+#include <wtf/StringExtras.h>

Ditto.

r- for now so that we can fix the vsnprintf and WIN_OS issues.