Bug 25972 - Help Disambiguate min/max calls in TimeRanges
: Help Disambiguate min/max calls in TimeRanges
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-22 18:06 PDT by Brent Fulgham
Modified: 2009-05-29 10:37 PDT (History)
0 users

See Also:


Attachments
Help compiler figure out arguments to min/max. (1.25 KB, patch)
2009-05-22 18:13 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Disambiguate min/max call. (1.25 KB, patch)
2009-05-22 18:16 PDT, Brent Fulgham
eric: review-
Details | Formatted Diff | Diff
Correct build error. (1.43 KB, patch)
2009-05-28 15:16 PDT, Brent Fulgham
darin: review-
Details | Formatted Diff | Diff
Revised per Darin's suggestions. (1.54 KB, patch)
2009-05-29 09:36 PDT, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2009-05-22 18:06:43 PDT
I am finding that the following change is needed to get a clean build:

$ diff TimeRanges.h TimeRanges.h.brent
93,94c93,94
<     ret.m_start = min(m_start, range.m_start);
<     ret.m_end = max(m_end, range.m_end);
---
>     ret.m_start = std::min(m_start, range.m_start);
>     ret.m_end = std::max(m_end, range.m_end);

It seems that some difference in the include order (or included files) allows the CG builds to handle this without needing to be told to use the std:: template call.
Comment 1 Brent Fulgham 2009-05-22 18:13:22 PDT
Created attachment 30599 [details]
Help compiler figure out arguments to min/max.
Comment 2 Brent Fulgham 2009-05-22 18:16:27 PDT
Created attachment 30600 [details]
Disambiguate min/max call.
Comment 3 Eric Seidel 2009-05-22 19:52:44 PDT
Comment on attachment 30600 [details]
Disambiguate min/max call.

This needs more explanation in the ChangeLog as to why this is necessary or correct.  Without that I can't tell.  Feel free to explain it to maciej or I (or any other reviewer) in teh bug or over IRC.
Comment 4 Brent Fulgham 2009-05-27 09:05:22 PDT
Building the Windows Cairo target, the min/max calls in TimeRanges.h are misinterpreted, leading to a compile-time error:
c:\Cygwin\home\bfulgham\WebKit\WebCore\html\TimeRanges.h(93) : error C2440: '=' : cannot convert from 'WebCore::SMILTime' to 'float'
        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
c:\Cygwin\home\bfulgham\WebKit\WebCore\html\TimeRanges.h(94) : error C2440: '=' : cannot convert from 'WebCore::SMILTime' to 'float'
        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

Somehow, it seems to be confused about the correct signature for min/max, resolving to the animation/SMILTime data type:
     inline SMILTime max(const SMILTime& a, const SMILTime& b) { return std::max(a.value(), b.value()); }
     inline SMILTime min(const SMILTime& a, const SMILTime& b) { return std::min(a.value(), b.value()); }

Modifying the TimeRanges implementation to explicitly use the std::min/std::max calls resolves the compile error.
Comment 5 Brent Fulgham 2009-05-28 15:16:59 PDT
Created attachment 30755 [details]
Correct build error.

Revise changelog to explain problem.
Comment 6 Darin Adler 2009-05-28 15:28:14 PDT
Comment on attachment 30755 [details]
Correct build error.

> +        Compiler mistakenly selects SMILTime min/max instead of STL version,

Why? What even includes SMILTime?
Comment 7 Brent Fulgham 2009-05-28 22:29:21 PDT
(In reply to comment #6)
> (From update of attachment 30755 [details] [review])
> > +        Compiler mistakenly selects SMILTime min/max instead of STL version,
> 
> Why? What even includes SMILTime?
> 

This is a byproduct of compiling the DerivedSources.cpp file, which includes and builds a ton of stuff.  Something in the 300-odd files included by DerivedSources.cpp causes SMILTime.h to be included, which sets us up for the problem.
Comment 8 Darin Adler 2009-05-29 07:53:15 PDT
Comment on attachment 30755 [details]
Correct build error.

This is close to correct, but not quite.

Since this is a header, it's not appropriate to say "using namespace std" here. We need to remove that. Adding the include of <algorithm> is also good, but it needs to be sorted in with the rest of the includes, not a separate paragraph. And we do indeed need to add the "std::" prefixes.

Further, SMILTime.h should not define min and max functions, since the std versions will work fine on SMILTime objects.

review- because I'd prefer to see a patch that removes the incorrect "using namespace std".
Comment 9 Brent Fulgham 2009-05-29 09:36:37 PDT
Created attachment 30776 [details]
Revised per Darin's suggestions.

Remove 'using namespace std', place "#include <algorithm>" in similar location as other files.
Comment 10 Adam Roben (:aroben) 2009-05-29 09:50:21 PDT
Comment on attachment 30776 [details]
Revised per Darin's suggestions.

r=me
Comment 11 Brent Fulgham 2009-05-29 10:37:34 PDT
Landed in r44265.