Bug 14967 – Reduce wtf::Vector::operator[]() overloads and eliminate unsafe auto-cast operator

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Peter Kasting]

Vector.h provides an at(size_t) accessor and a series of operator[]()s, all of which call at(), but none of which take a size_t.  On Windows 64-bit, where size_t is a 64-bit type, this results in compiler warnings when calling Vector[size_t].

It used to be the case that operator[]() _did_ take a size_t, and, ironically, this was changed to eliminate Windows compiler warnings.  But the change was the wrong fix: the actual problem at the time (I think, from inspection of the original source) was that there was also an overload which took an int, meaning that promotion from shorter types was ambiguous.

Inspection of the STL headers suggested that providing only a size_t version should always be safe (which makes sense; it should be at least as large a type as almost any parameter a caller would actually provide, so autopromotion should work OK).  After writing an initial patch to do this, I discovered that another, scary operator was causing overload resolution ambiguity: operator T*().  This allows callers to treat Vectors directly as pointers.  I consider this scary since it can result in silent conversion to bool in ways callers don't expect.  On IRC, Maciej also noted that this operator was "questionable".

So, I have a patch that eliminates this operator as well, and changes all callers who depended on it to doing the right thing (which usually meant adding a call to ".data()").  This shouldn't make any code slower as operator T* was just an inline call to data() anyway.

I have successfully compiled this patch on Windows without any compiler warnings.  Still need someone to test it on Mac.  Will attach the patch shortly.

[Peter Kasting]

Created attachment 15969 [details]
patch v1

No ChangeLog in this patch because for some reason the WebKit scripts are giving me problems with DOS vs. UNIX line endings.

[Peter Kasting]

Created attachment 15970 [details]
patch v2

Somehow I seem to have missed one of the spots in FrameView.cpp without getting warned about it...

[Darin Adler]

Comment on attachment 15969 [details]
patch v1

-            if (!plugin->mimes)
+            if (plugin->mimes.isEmpty())

This looks like an actual minor bug fix, not just a compile fix.

-    if (!d->m_scheduledEvents)
+    if (d->m_scheduledEvents.isEmpty())
         return;

As does this.

The WebKitTools/Scripts/svn-create-patch change seems good, but doesn't belong in this patch.

The safest way to land this change is to land all the obvious .data() changes first, then look more closely at the smaller remaining bits.

I'd like to hear Maciej's thoughts on getting rid of the default conversion functions. I was never a fan of them.

[Peter Kasting]

Created attachment 15971 [details]
patch v3

This version of the patch should compile on a Mac as well.

[Peter Kasting]

Created attachment 15972 [details]
partial (safe) patch v1

Sorry about the svn-create-patch changes... hadn't meant that to be involved in this bug at all.  Oops.

Per Darin's suggestion, this patch should hopefully contain just changes that add ".data()", which was the majority of the original patch.  This should be easy to quickly r+ and land, and then I can attach a much smaller patch that's the more review-worthy stuff.

[Darin Adler]

Comment on attachment 15972 [details]
partial (safe) patch v1

r=me

[Peter Kasting]

Since I'm having problems with prepare-ChangeLog ATM, the relevant details for someone landing this are:

patch by Peter Kasting <[e-mail]>, reviewed by Darin
http://bugs.webkit.org/show_bug.cgi?id=14967 part 1 - Eliminate most implicit conversions of wtf::Vector<T> to T* by explicitly calling .data().

[Andrew Wellington]

Comment on attachment 15972 [details]
partial (safe) patch v1

Removing review flag (patch landed in r25085)

[Peter Kasting]

Created attachment 15974 [details]
Remaining patch v1

Now that the large, obviously safe portion of this patch has landed, here's the remainder of the original v3 patch.

[Peter Kasting]

Created attachment 15997 [details]
Remove all remaining bits of operator T*

At Maciej's request, splitting the previous final patch into two pieces, this one to complete removal of the auto-cast operator T*(), and an upcoming one to do the then-safe reduction of the operator[]() overloads.

[Peter Kasting]

Created attachment 15998 [details]
Reduce operator[] overloads

This is safe to land once the patch above it lands.

[Sam Weinig]

Comment on attachment 15997 [details]
Remove all remaining bits of operator T*

Looks good. r=me.

[Sam Weinig]

(In reply to comment #12)
> (From update of attachment 15997 [details] [edit])
> Looks good. r=me.
> 

It does need a ChangeLog entry though.

[Mark Rowe (bdash)]

Comment on attachment 15998 [details]
Reduce operator[] overloads

Have you tested this change on the Mac to verify that no warnings are introduced?  A ChangeLog entry is also needed.

[Mark Rowe (bdash)]

Comment on attachment 15997 [details]
Remove all remaining bits of operator T*

Marking r- for now as this can't be landed without a changelog.

[Peter Kasting]

Comment on attachment 15997 [details]
Remove all remaining bits of operator T*

I tested on a friend's Mac to ensure that the compile succeeded.  I wasn't really able to see whether any new warnings were issued.  If someone else could check that that would be great.  That said, this is how the STL does things, so I suspect it's OK.

As mentioned previously, I'm having problems getting prepare-ChangeLog to work for me.  Here's a ChangeLog entry someone landing this patch could use:

patch by Peter Kasting <[e-mail]>, reviewed by Sam Weinig
http://bugs.webkit.org/show_bug.cgi?id=14967 part 2 - Eliminate all remaining
implicit conversions of wtf::Vector<T> to T*.  Where code was previously checking that the Vector's data pointer was non-NULL, check !Vector::isEmpty() instead.

[Mark Rowe (bdash)]

What's wrong with prepare-ChangeLog?  It's working fine for everyone else on Windows, Mac and Linux using both SVN and git.

[Peter Kasting]

It's having trouble with my particular corporate Cygwin/SVN setup, which has a lot of DOS CR/LFs creeping into places where it only expects LFs.  I have a local patch that fixes this for svn-create-patch (which accidentally snuck into the "patch v3" attachment above), but I haven't figured out the equivalent spot to path in prepare-changeLog.

Basically, it results in the filenames passed to svn having extra CRs on the ends, which leads to svn being confused and aborting.

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> It's having trouble with my particular corporate Cygwin/SVN setup, which has a
> lot of DOS CR/LFs creeping into places where it only expects LFs.

Sounds like the CR/LF line endings setting for Cygwin is different than expected (for the Perl scripts).

[Adam Roben (:aroben)]

Comment on attachment 15997 [details]
Remove all remaining bits of operator T*

Did you do any testing to make sure that non-Mac platforms still build with this patch?

If so, r=me

[Peter Kasting]

I've tested Windows and Mac, but I don't have the ability to build other platforms beyond that.

[Maciej Stachowiak]

Comment on attachment 15998 [details]
Reduce operator[] overloads

r=me

Anyone landing this please note ChangeLog entry in bug comments.

[Peter Kasting]

Resummarizing

[Mark Rowe (bdash)]

Landed in r25240 and r25241.