Summary: | Improve visual studio 2008 sp1 compilation | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marc-Antoine Ruel <maruel> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Attachments: |
|
Description
Marc-Antoine Ruel
2008-11-16 16:40:33 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.
This patch builds and run on Mac OS X 10.5.5 too. 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.
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. CCing our template gurus. Created attachment 25225 [details]
Fixes compilation issues with VS2008
Same as previous with ChangeLog updated.
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 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 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.
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. :( |