Bug 14206 – MSVC7 support and PLATFORM define fixes for Windows

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kevin Ollivier]

This patch adds support for building on MSVC7, and also fixes a few incorrect PLATFORM defines so that they will work with any port that builds a Windows version.

[Kevin Ollivier]

Created attachment 15094 [details]
MSVC7 support and fixes for PLATFORM defines

[Darin Adler]

Comment on attachment 15094 [details]
MSVC7 support and fixes for PLATFORM defines

A lot of these things look like library differences rather than compiler differences. Should we really be using a COMPILER check for them?

+#if _MSC_VER >= 1400
+#define WTF_COMPILER_MSVC8 1
+#else
+/* assumes everything else is like MSVC7 */
+#define WTF_COMPILER_MSVC7 1
+#endif

I'd prefer the logic be reversed. The past should be the exceptional case, and the future should be the normal case. But anyway, I don't think the new thing should be called MSVC8 -- we don't want any MSVC8 conditionals in the code until we know how it differs from MSVC9, for example.

[Kevin Ollivier]

I could go ahead and refactor the MSVC7 define block to read:

#if _MSC_VER < 1400
/* assumes everything < MSVC8 is like MSVC7 */
#define WTF_COMPILER_MSVC7 1
#endif

Would that work?

I wasn't really sure what you meant about library rather than compiler differences though. Are you suggesting we test for MSVC7-only functions and support in another way, or are you suggesting we always use the MSVC7 variant of the function?

[Maciej Stachowiak]

Comment on attachment 15094 [details]
MSVC7 support and fixes for PLATFORM defines

Yes, please redo the conditionals like you said. Also, for the vsnprintf changes, could you please make a wrapper of some kind (maybe just #define vsnprintf _vsnprintf for the relevant compiler) instead of putting ifdefs at every call site?

We prefer to avoid platform ifdefs in core code.

(Side not to Darin: on Windows the C library is distributed with the dev tools, not with the OS, so I think the COMPILER checks are ok.)

Please revise.

[Kevin Ollivier]

Created attachment 16561 [details]
New patch with changes made

Sorry it took so long, I've been busy and haven't had much time for WebKit work over the past couple months. All the changes you requested should be in the new patch. I checked it with latest SVN and the JavaScriptCore tests pass okay, but I haven't been able to get the layout tests working on my setup, with or without this patch, so I haven't been able to confirm with the layout tests.

[Adam Roben (:aroben)]

Comment on attachment 16561 [details]
New patch with changes made

 #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.

[Kevin Ollivier]

Created attachment 16577 [details]
Patch with Adam Roben's changes

Thanks for taking a look at this! I think you're right that the debugger code should be checking for the MSVC compiler. Whether or not the code can compile in other environments (which I'm not sure about either), it's only useful when you plan to use it with the Visual Studio IDE anyway, so I think it's best to define it this way. As for vsnprintf, they weren't the same in previous iterations which is why I had separate functions. I just didn't pay attention to the fact that my recent changes made them the same. :-)

[Adam Roben (:aroben)]

Comment on attachment 16577 [details]
Patch with Adam Roben's changes

r=me (I assume marking this r- was a mistake when you uploaded the patch).

[Mark Rowe (bdash)]

Landed in r26589.