Bug 22299 - Improve visual studio 2008 sp1 compilation
Summary: Improve visual studio 2008 sp1 compilation
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-16 16:40 PST by Marc-Antoine Ruel
Modified: 2008-11-17 15:35 PST (History)
0 users

See Also:


Attachments
Template fixes (3.45 KB, patch)
2008-11-16 16:47 PST, Marc-Antoine Ruel
eric: review-
Details | Formatted Diff | Diff
Fixes compilation issues with VS2008 (3.96 KB, patch)
2008-11-17 13:41 PST, Marc-Antoine Ruel
maruel: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-Antoine Ruel 2008-11-16 16:40:33 PST
Fixes some compilation issues with Visual Studio 2008 Service Pack 1.

There is still issues with the link step. At least, the compile step now pass.
Comment 1 Marc-Antoine Ruel 2008-11-16 16:47:46 PST
Created attachment 25203 [details]
Template fixes

VS2008 refuses to have a specialization of a function template as a friend. It can only mark as a friend the unspecialized template.

To fix this, a different typename must be used for the function template description. I just added an underscore to the typenames.
Comment 2 Marc-Antoine Ruel 2008-11-17 07:46:33 PST
This patch builds and run on Mac OS X 10.5.5 too.
Comment 3 Eric Seidel (no email) 2008-11-17 13:29:21 PST
Comment on attachment 25203 [details]
Template fixes

You should set CHANGE_LOG_NAME or REAL_NAME to "Marc-Antoine Ruel" (or just edit the ChangeLog after the fact to have your real name instead of maruel).

Why add _ to the end of these template parameter names?  That's not WebKit style, it might be google style, I'm not even sure of that (but that doesn't really have any bearing here).

Also generally we mention the bug number in the changelog, and it would be nice for documentations sake to have the text of the warning/error in the bug or the changelog.  Maciej or Darin (Adler) would be most likely to review this change, as they're the two template experts in the WebKit community (that I know of).

r- for the reasons mentioned above.
Comment 4 Eric Seidel (no email) 2008-11-17 13:30:53 PST
Pasting the error is not really necessary in the bug.  You've explained the bug nicely above:

VS2008 refuses to have a specialization of a function template as a friend. It
can only mark as a friend the unspecialized template.

To fix this, a different typename must be used for the function template
description.

But you should add that (or similar) to the ChangeLog, since the ChangeLog becomes the commit log and is the most accessible record.
Comment 5 Eric Seidel (no email) 2008-11-17 13:31:09 PST
CCing our template gurus.
Comment 6 Marc-Antoine Ruel 2008-11-17 13:41:34 PST
Created attachment 25225 [details]
Fixes compilation issues with VS2008

Same as previous with ChangeLog updated.
Comment 7 Sam Weinig 2008-11-17 14:04:07 PST
Comment on attachment 25225 [details]
Fixes compilation issues with VS2008

We don't, nor do we plan to in the short term, support Visual Studio 2008.  Therefore, I don't really see any reason to add these changes.
Comment 8 Eric Seidel (no email) 2008-11-17 14:04:22 PST
Comment on attachment 25225 [details]
Fixes compilation issues with VS2008

Marc and I talked this out over IM.  The change looks fine.  I'm still slightly confused as to why the type names need to be different in the forward declaration.

He pointed out that:
http://msdn.microsoft.com/en-us/library/bb384871.aspx
documents this error.

I would like either Maciej or Darin (Adler) to glance at this before one of us lands it though.
Comment 9 Sam Weinig 2008-11-17 14:18:02 PST
Comment on attachment 25225 [details]
Fixes compilation issues with VS2008

Based on popular demand, I am reverting my r-.  I still don't think there is a compelling reason to take this patch if we don't plan on officially supporting Visual Studio 2008, but I will leave the review status to wiser people than I.
Comment 10 Marc-Antoine Ruel 2008-11-17 15:35:00 PST
Ok, after the back and forth, I decided to verify if this fix was indeed for 3772 (error) or 4396 (warning).  I had made the patch a few months ago so I had assumed I was fixing an error. After testing, I just realized this patch fixes 4396.

So don't bother anymore. :(