Bug 10765 – Windows build busted due to std::copy usage in Vector.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Fisher (:fishd, Google)]

Windows build busted due to std::copy usage in Vector.h

Here are the build errors in all their glory:

C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : error C2220: warning treated as error - no 'object' file generated
C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : warning C4996: 'std::_Copy_opt' was declared deprecated
        C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2270) : see declaration of 'std::_Copy_opt'
        Message: 'You have used a std:: construct that is not safe. See documentation on how to use the Safe Standard C++ Library'
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(447) : see reference to function template instantiation '_OutIt std::copy<const WebCore::StyleDashboardRegion*,WebCore::StyleDashboardRegion*>(_InIt,_InIt,_OutIt)' being compiled
        with
        [
            _OutIt=WebCore::StyleDashboardRegion *,
            _InIt=const WebCore::StyleDashboardRegion *
        ]
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(436) : while compiling class template member function 'WTF::Vector<T> &WTF::Vector<T>::operator =(const WTF::Vector<T> &)'
        with
        [
            T=WebCore::StyleDashboardRegion
        ]
        c:\builds\webkit\webcore\rendering\RenderStyle.h(737) : see reference to class template instantiation 'WTF::Vector<T>' being compiled
        with
        [
            T=WebCore::StyleDashboardRegion
        ]
WebFrame.cpp
C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : error C2220: warning treated as error - no 'object' file generated
C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : warning C4996: 'std::_Copy_opt' was declared deprecated
        C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2270) : see declaration of 'std::_Copy_opt'
        Message: 'You have used a std:: construct that is not safe. See documentation on how to use the Safe Standard C++ Library'
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(447) : see reference to function template instantiation '_OutIt std::copy<const WebCore::StyleDashboardRegion*,WebCore::StyleDashboardRegion*>(_InIt,_InIt,_OutIt)' being compiled
        with
        [
            _OutIt=WebCore::StyleDashboardRegion *,
            _InIt=const WebCore::StyleDashboardRegion *
        ]
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(436) : while compiling class template member function 'WTF::Vector<T> &WTF::Vector<T>::operator =(const WTF::Vector<T> &)'
        with
        [
            T=WebCore::StyleDashboardRegion
        ]
        c:\builds\webkit\webcore\rendering\RenderStyle.h(737) : see reference to class template instantiation 'WTF::Vector<T>' being compiled
        with
        [
            T=WebCore::StyleDashboardRegion
        ]
WebView.cpp
C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : error C2220: warning treated as error - no 'object' file generated
C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2282) : warning C4996: 'std::_Copy_opt' was declared deprecated
        C:\Program Files\Microsoft Visual Studio 8\VC\include\xutility(2270) : see declaration of 'std::_Copy_opt'
        Message: 'You have used a std:: construct that is not safe. See documentation on how to use the Safe Standard C++ Library'
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(447) : see reference to function template instantiation '_OutIt std::copy<const WebCore::StyleDashboardRegion*,WebCore::StyleDashboardRegion*>(_InIt,_InIt,_OutIt)' being compiled
        with
        [
            _OutIt=WebCore::StyleDashboardRegion *,
            _InIt=const WebCore::StyleDashboardRegion *
        ]
        c:\builds\WebKit\JavaScriptCore\wtf/Vector.h(436) : while compiling class template member function 'WTF::Vector<T> &WTF::Vector<T>::operator =(const WTF::Vector<T> &)'
        with
        [
            T=WebCore::StyleDashboardRegion
        ]
        c:\builds\webkit\webcore\rendering\RenderStyle.h(737) : see reference to class template instantiation 'WTF::Vector<T>' being compiled
        with
        [
            T=WebCore::StyleDashboardRegion
        ]

[Darin Fisher (:fishd, Google)]

Some more info about std::copy here:
http://lists.boost.org/Archives/boost/2005/07/89697.php

[Darin Fisher (:fishd, Google)]

Created attachment 10437 [details]
crappy patch to disable warning in certain source files

This patch is just a stop-gap measure to get things compiling again.

[Darin Fisher (:fishd, Google)]

Created attachment 10444 [details]
alternatve patch: no pragmas

This patch solves the problem a different way.  It just avoids std::copy altogether.  Note: for basic types that support memcpy, this is probably better since it replaces a for-loop + memcpy with a single memcpy.  For more complex types, it means an extra pass over the array, which might hurt locality of reference, so perhaps that's reason to poo-poo this patch :-/

[Darin Adler]

Comment on attachment 10444 [details]
alternatve patch: no pragmas

I really like this approach.

But since std::copy is really just a while loop, there's no reason to change the semantics and possibly the performance characteristics just to get rid of std::copy.

We could just create a couple local variables and write the loop by hand to avoid the warning. Or write our own equivalent of the std::function.

I suggest we consider changing to the uninitializedCopy approach on its own merits rather than doing it as part of this change.

I'm going to mark this review- because of that issue, even though I really like the direction of this fix.

[Darin Fisher (:fishd, Google)]

So, you would prefer a separate bug?  And, you would prefer a WTF::copy?  Or, should this be a TypeOperations::copy?

[Alexey Proskuryakov]

In STL implementations that I'm familiar with (GCC, CodeWarrior), std::copy is not simply a while loop. Its implementation is in fact heavily overloaded:
- memmove for built-in and POD types (using compiler-specific ways to detect PODs);
- specialized algorithms for copying data in known containers (e.g., chunked copy for deques in MSL);
- a for loop for random access iterators in GCC (helps the optimizer);
- a while loop otherwise.

To duplicate the behavior for PODs, we'd need to introduce quite a bit of compiler-specific code.

[Karthik Kumar]

Why not simply:

...

        clear();
        reserveCapacity(other.size());
        
	  std::copy(other.begin(), other.begin() + size(), begin());

..

[Mark Rowe (bdash)]

After a bit of reading I have discovered that the warning is due to to Microsoft's "Safe Standard C++ Library" (http://msdn2.microsoft.com/en-US/library/ms404581.aspx), in particular the explicit use of "Checked Iterators" (http://msdn2.microsoft.com/en-US/library/y9ww7c1a.aspx) which involves the use of unchecked iterators emitting a compiler warning about deprecation.

I'm not convinced that we should work around Microsofts deprecation of a standard library function by changes in the code.  Their "Checked Iterators" page (http://msdn2.microsoft.com/en-US/library/y9ww7c1a.aspx) two preprocessor constants that can be used to control the use of checked iterators.  Defining _SECURE_SCL_NO_DEPRECATE will disable the compile-time warnings that checked iterators can emit, and defining _SECURE_SCL to 0 will disable the use of checked iterators entirely.

With regards to Karthik's comment -- further discussion on IRC concluded that the body of operator= could be changed to:

        if (&other == this)
            return *this;

        clear();
        reserveCapacity(other.size());
        TypeOperations::uninitializedCopy(other.begin(), other.end(), begin());
        m_size = other.size();

        return *this;

to retains the existing semantics while removing the call to std::copy.  It does change the performance characteristics somewhat in the case that size() > other.size() || other.size() < capacity(), but I'm not sure as to what extent.

[Darin Fisher (:fishd, Google)]

We can also try to make use of stdext::unchecked_copy when building on Windows.

[Darin Fisher (:fishd, Google)]

Created attachment 10504 [details]
yet another patch: w/ replacement for std::copy

Here's a patch that implements and uses TypeOperations::copy instead of std::copy.  OK?

[Alexey Proskuryakov]

(In reply to comment #8)
> I'm not convinced that we should work around Microsofts deprecation of a
> standard library function by changes in the code.

I can only second that. The "deprecation" warnings are there to help with adopting Microsoft's checked iterators - not something we are likely to do, as far as I can tell.

[Darin Fisher (:fishd, Google)]

So, do you guys prefer a patch that disables the warnings altogether instead?

(By the way, it sucks that the build has been broken for so long because of this problem.)

[Mark Rowe (bdash)]

It seems that Alexey and myself would both prefer that.  I would have provided a patch myself, but as I don't use Windows I have no means to create one.  These problems don't just fix themselves :-)

[Darin Fisher (:fishd, Google)]

OK, can you and Darin come to some agreement?  (I don't know who gets to decide what the right solution is, but you guys don't seem to be in agreement!)  I'm happy to provide a patch for whatever solution will be accepted :-)

[Darin Fisher (:fishd, Google)]

Created attachment 10508 [details]
alternate patch: define _SCL_SECURE_NO_DEPRECATE to kill warning

OK, this patch implements the suggested vcproj change.

[Adam Roben (:aroben)]

Comment on attachment 10508 [details]
alternate patch: define _SCL_SECURE_NO_DEPRECATE to kill warning

r=me. This will get the build working, and we can discuss the merits of the other approach later. Thanks!

[Adam Roben (:aroben)]

Comment on attachment 10508 [details]
alternate patch: define _SCL_SECURE_NO_DEPRECATE to kill warning

Landed as r16317.