Bug 38630

Created attachment 55211 [details] Patch Fixes the assert.

It's not good for performance to add another branch. Perhaps this should only be done when building with VS2010? And a bug should probably be filed with Microsoft.

AFAIK, passing 0 to an Iterator parameter is invalid in C++. So this behavior of VS2010 seems reasonable. If we worry about the performance, we should use "#if COMPILER(MSVC)" here.

I think we should add just } else if (!other.size()) return *this; (No removing "if (!begin()) return *this;") This is an optimization for all compilers and solve the VS2010 issue.

I do not see any restrictions on input or output iterators in copy() algorithm description in C++ standard. Do you have a reference for this requirement? > This is an optimization for all compilers and solve the VS2010 issue. That also sounds like an extra branch in common case. Or did I misunderstand your proposal?

(In reply to comment #5) > I do not see any restrictions on input or output iterators in copy() algorithm > description in C++ standard. Do you have a reference for this requirement? I checked the C++0x draft, and I think i was wrong. 0 can be a valid iterator in general. The standard says neither "The OutputIterator parameter must be dereferenceable" nor "It may be not dereferenceable if [first, last) is empty." So we can't assume the VS2010 behavior is a bug. > > This is an optimization for all compilers and solve the VS2010 issue. > > That also sounds like an extra branch in common case. Or did I misunderstand your proposal? Yes, it will make an extra branch. But it will skip the following 3-lines of the code in a case of empty vectors. I have no strong preference on it. I think either of it or "#if COMPILER(MSVC)" are ok.

Created attachment 55543 [details] Kent's proposal patch Here is a patch for the proposed fix. This adds a branch when other.size() is non-zero and fits in capacity(), but exit earlier in other cases. I would also be ok with wrapping the previous patch in a MSVC/MSVC2010 specific macro piece if you think this one has issues. Anyhow the msvc behavior, being a bug or not, has been reported there: https://connect.microsoft.com/VisualStudio/feedback/details/558044/std-copy-should-not-check-dest-when-first-last

Attachment 55543 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/Vector.h:680: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] JavaScriptCore/wtf/Vector.h:705: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 55545 [details] Kent's proposal patch v2 Fix style issues

Comment on attachment 55545 [details] Kent's proposal patch v2 > @@ -677,14 +677,17 @@ namespace WTF { > if (&other == this) > return *this; > > - if (size() > other.size()) > + if (size() > other.size()) { > shrink(other.size()); > - else if (other.size() > capacity()) { > + return *this; > + } This branch just resizes 'this' and doesn't copy the content of 'other'.

Comment on attachment 55545 [details] Kent's proposal patch v2 > The standard says neither "The OutputIterator parameter must be dereferenceable" > nor "It may be not dereferenceable if [first, last) is empty." So we can't > assume the VS2010 behavior is a bug. The standard doesn't need to explicitly mention everything it allows. If there is no restriction, then it's allowed. Please make this fix only for MSVC2010, file a bug with them, and reference this bug in a comment in code, so that we could know when to remove the workaround. - if (size() > other.size()) + if (size() > other.size()) { shrink(other.size()); - else if (other.size() > capacity()) { + return *this; + } This doesn't look like it could possibly work. Did you run regression tests with this patch? Or am I just confused?

> This branch just resizes 'this' and doesn't copy the content of 'other'. Sorry, didn't see your comment before reviewing. So, I'm not just confused :-)

Created attachment 55682 [details] Patch v2 (In reply to comment #11) > This doesn't look like it could possibly work. Did you run regression tests with this patch? Or am I just confused? No, there was a lack of testing from my part on this patch, sorry about that. Here is the first patch enclosed to apply only when the assert would pop. Adding only "else if (!other.size()) return *this;" would not prevent the assert when size() > other.size() and the buffer gets shrunk to 0. See Comment #7 for a link to the reported visual studio bug.

Comment on attachment 55682 [details] Patch v2 +// Workarounds an assert in VS2010. See https://bugs.webkit.org/show_bug.cgi?id=38630 There is no verb "workarounds", it's either a noun or "works around". I still think that it would be better to have a direct link to MS issue. The use case for this comment is: can I remove this code now? - click link - read. With a Bugzilla link, there is an extra step involved. On the other hand, the Bugzilla link may be more stable. r=me, but please fix the grammar issue.

Committed r59463: <http://trac.webkit.org/changeset/59463> Changes since the review: - Fixed the grammar issue - The link in the code points to the VS issue instead - Changed the preprocessor expression to also match when _ITERATOR_DEBUG_LEVEL == 1 thanks!

Revision r59463 cherry-picked into qtwebkit-2.0 with commit a1a9dda8011dbd6ffc40bad8b4e62d2d3bf95916

Update: The bug has been fixed in Visual Studio and the fix will be available in VS2011. http://connect.microsoft.com/VisualStudio/feedback/details/558044/std-copy-should-not-check-dest-when-first-last

(In reply to comment #17) > The bug has been fixed in Visual Studio and the fix will be available in VS2011. err.. I mean VC11