Bug 14967 - Reduce wtf::Vector::operator[]() overloads and eliminate unsafe auto-cast operator
Summary: Reduce wtf::Vector::operator[]() overloads and eliminate unsafe auto-cast ope...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-14 13:06 PDT by Peter Kasting
Modified: 2007-08-25 07:08 PDT (History)
2 users (show)

See Also:


Attachments
patch v1 (18.99 KB, patch)
2007-08-14 13:54 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v2 (19.28 KB, patch)
2007-08-14 14:43 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v3 (21.13 KB, patch)
2007-08-14 15:45 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
partial (safe) patch v1 (15.70 KB, patch)
2007-08-14 15:51 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Remaining patch v1 (4.84 KB, patch)
2007-08-14 19:54 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Remove all remaining bits of operator T* (3.98 KB, patch)
2007-08-16 11:04 PDT, Peter Kasting
aroben: review+
Details | Formatted Diff | Diff
Reduce operator[] overloads (1.19 KB, patch)
2007-08-16 11:05 PDT, Peter Kasting
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2007-08-14 13:06:02 PDT
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.
Comment 1 Peter Kasting 2007-08-14 13:54:58 PDT
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.
Comment 2 Peter Kasting 2007-08-14 14:43:44 PDT
Created attachment 15970 [details]
patch v2

Somehow I seem to have missed one of the spots in FrameView.cpp without getting warned about it...
Comment 3 Darin Adler 2007-08-14 15:04:42 PDT
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.
Comment 4 Peter Kasting 2007-08-14 15:45:03 PDT
Created attachment 15971 [details]
patch v3

This version of the patch should compile on a Mac as well.
Comment 5 Peter Kasting 2007-08-14 15:51:49 PDT
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.
Comment 6 Darin Adler 2007-08-14 15:56:52 PDT
Comment on attachment 15972 [details]
partial (safe) patch v1

r=me
Comment 7 Peter Kasting 2007-08-14 17:20:58 PDT
Since I'm having problems with prepare-ChangeLog ATM, the relevant details for someone landing this are:

patch by Peter Kasting <zerodpx@gmail.org>, 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().
Comment 8 Andrew Wellington 2007-08-14 18:15:02 PDT
Comment on attachment 15972 [details]
partial (safe) patch v1

Removing review flag (patch landed in r25085)
Comment 9 Peter Kasting 2007-08-14 19:54:21 PDT
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.
Comment 10 Peter Kasting 2007-08-16 11:04:35 PDT
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.
Comment 11 Peter Kasting 2007-08-16 11:05:30 PDT
Created attachment 15998 [details]
Reduce operator[] overloads

This is safe to land once the patch above it lands.
Comment 12 Sam Weinig 2007-08-17 23:20:53 PDT
Comment on attachment 15997 [details]
Remove all remaining bits of operator T*

Looks good. r=me.
Comment 13 Sam Weinig 2007-08-17 23:24:01 PDT
(In reply to comment #12)
> (From update of attachment 15997 [details] [edit])
> Looks good. r=me.
> 

It does need a ChangeLog entry though.
Comment 14 Mark Rowe (bdash) 2007-08-17 23:38:27 PDT
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.
Comment 15 Mark Rowe (bdash) 2007-08-17 23:40:56 PDT
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.
Comment 16 Peter Kasting 2007-08-18 00:17:24 PDT
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 <zerodpx@gmail.org>, 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.
Comment 17 Mark Rowe (bdash) 2007-08-18 00:21:36 PDT
What's wrong with prepare-ChangeLog?  It's working fine for everyone else on Windows, Mac and Linux using both SVN and git.
Comment 18 Peter Kasting 2007-08-18 00:33:50 PDT
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.
Comment 19 David Kilzer (:ddkilzer) 2007-08-18 14:30:09 PDT
(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).

Comment 20 Adam Roben (:aroben) 2007-08-18 16:57:50 PDT
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
Comment 21 Peter Kasting 2007-08-18 19:30:46 PDT
I've tested Windows and Mac, but I don't have the ability to build other platforms beyond that.
Comment 22 Maciej Stachowiak 2007-08-19 13:11:19 PDT
Comment on attachment 15998 [details]
Reduce operator[] overloads

r=me

Anyone landing this please note ChangeLog entry in bug comments.
Comment 23 Peter Kasting 2007-08-23 11:17:02 PDT
Resummarizing
Comment 24 Mark Rowe (bdash) 2007-08-25 07:08:59 PDT
Landed in r25240 and r25241.