Bug 47063 – Fix broken C++ in PODInterval and PODIntervalTree

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

Fix broken C++ in PODInterval and PODIntervalTree

[Nico Weber]

Created attachment 69592 [details]
Patch

[Nico Weber]

kbr: Please review.

evan, hans: fyi

[WebKit Review Bot]

Attachment 69592 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4162058

[Nico Weber]

dglazkov: Which gcc version is cr-linux running? Builds fine for me locally with 4.0.1 and 4.2.1.

[Nico Weber]

Turns out function templates can't be partially specialized. Please refrain from reviewing…

[Nico Weber]

Thinking about this, the options seem to be:

1. Convert the function template into a class template, which can then use partial specialization like this:
template <class T>
struct ValueToString {
    static String value(const T& t);
};
template <>
struct<int> ValueToString {
    static String value(const int& t) { return intToStr(t); }
};

etc.


2. Use argument-dependent lookup for the function. ADL only works for class types, so I'd need to declare the function for all basic types, and could then use ADL for the rest.


3. Just delete this code. It's only used for debugging and probably isn't worth all the complexity.


I think we should go with 3. Comments?

[Kenneth Russell]

(In reply to comment #6)
> Thinking about this, the options seem to be:
> 
> 1. Convert the function template into a class template, which can then use partial specialization like this:
> template <class T>
> struct ValueToString {
>     static String value(const T& t);
> };
> template <>
> struct<int> ValueToString {
>     static String value(const int& t) { return intToStr(t); }
> };
> 
> etc.
> 
> 
> 2. Use argument-dependent lookup for the function. ADL only works for class types, so I'd need to declare the function for all basic types, and could then use ADL for the rest.
> 
> 
> 3. Just delete this code. It's only used for debugging and probably isn't worth all the complexity.
> 
> 
> I think we should go with 3. Comments?

I do not support simply deleting this code. It has occasionally proven essential for debugging purposes. The original code used iostreams and overloaded operator<< to provide printing functionality. Either option (1) or (2) sounds fine. I'm happy to make the change to (1) if you want.

[Nico Weber]

Created attachment 69644 [details]
Patch

[Nico Weber]

Approach 1 it is.

[WebKit Review Bot]

Attachment 69644 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4173065

[Nico Weber]

Created attachment 69664 [details]
Patch

[Kenneth Russell]

Comment on attachment 69664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69664&action=review

Code changes look fine; please note minor documentation changes needed.

> WebCore/platform/graphics/gpu/PODInterval.h:64
> +//       String string(const T& t);

This should be documented as being static, as is used in the code.

> WebCore/platform/graphics/gpu/PODInterval.h:67
> +//       String string(const UserData& t);

static.

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:48
> +//       String string(const T& t);

static.

[Nico Weber]

Created attachment 69677 [details]
Patch

[Kenneth Russell]

Comment on attachment 69677 [details]
Patch

Looks fine. I think you need to wait for the r+ before marking it cq+, though.

[WebKit Commit Bot]

Comment on attachment 69677 [details]
Patch

Clearing flags on attachment: 69677

Committed r69081: <http://trac.webkit.org/changeset/69081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.