Summary: | Help Disambiguate min/max calls in TimeRanges | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | ||||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Created attachment 30599 [details]
Help compiler figure out arguments to min/max.
Created attachment 30600 [details]
Disambiguate min/max call.
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.
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. Created attachment 30755 [details]
Correct build error.
Revise changelog to explain problem.
Comment on attachment 30755 [details] Correct build error. > + Compiler mistakenly selects SMILTime min/max instead of STL version, Why? What even includes SMILTime? (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 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".
Created attachment 30776 [details]
Revised per Darin's suggestions.
Remove 'using namespace std', place "#include <algorithm>" in similar location as other files.
Comment on attachment 30776 [details]
Revised per Darin's suggestions.
r=me
Landed in r44265. |
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.