Bug 47646

Summary:

Add guideline for constructors doing implicit type conversion to coding style.

Product:

WebKit

Reporter:

David Levin <levin>

Component:

WebKit Website

Assignee:

David Levin <levin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mjs

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed fix.	levin: review-
Proposed fix.	darin: review+, levin: commit-queue-

David Levin

Reported 2010-10-13 20:06:12 PDT

The text is meant to follow the consensus from https://lists.webkit.org/pipermail/webkit-dev/2010-September/014523.html

Attachments
Proposed fix. (1.41 KB, patch) 2010-10-13 20:22 PDT, David Levin	levin: review-	Details Formatted Diff Diff
Proposed fix. (1.44 KB, patch) 2010-10-14 02:55 PDT, David Levin	darin: review+ levin: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

David Levin

Comment 1 2010-10-13 20:22:33 PDT

Created attachment 70703 [details] Proposed fix.

Adam Barth

Comment 2 2010-10-13 20:30:33 PDT

Comment on attachment 70703 [details] Proposed fix. Looks good to me. We might wait to see if Darin has comments.

Maciej Stachowiak

Comment 3 2010-10-13 21:00:06 PDT

I'd suggest incorporating further comments, that sometimes instead of an explicit constructor you may not want a constructor at all, but perhaps a function returning the type instead. As Darin put it, the rule is really about when it is or isn't ok to have a constructor that creates an implicit type conversion, rather than about when to use explicit. The explicit keyword is one of the possible solutions if there is an implicit type conversion but there shouldn't be.

David Levin

Comment 4 2010-10-14 02:42:53 PDT

Comment on attachment 70703 [details] Proposed fix. Marking r- per mjs comment.

David Levin

Comment 5 2010-10-14 02:55:06 PDT

Created attachment 70722 [details] Proposed fix.

Darin Adler

Comment 6 2010-10-14 16:31:32 PDT

Comment on attachment 70722 [details] Proposed fix. Might be worthwhile to mention that we also do not put in the meaningless explicit keyword when there are no arguments or more than one non-default argument. I see incorrect uses of explicit in files such as BindingSecurity.h, JSDOMWrapper.h, JSMainThreadExecState.h, ScheduledAction.h, V8BindingState.h, Canvas2DLayerChromium.h, RenderThemeQt.h, IDBCursor.h.

David Levin

Comment 7 2010-10-14 22:18:13 PDT

(In reply to comment #6) > (From update of attachment 70722 [details]) > Might be worthwhile to mention that we also do not put in the meaningless explicit keyword when there are no arguments or more than one non-default argument. I see incorrect uses of explicit in files such as BindingSecurity.h, JSDOMWrapper.h, JSMainThreadExecState.h, ScheduledAction.h, V8BindingState.h, Canvas2DLayerChromium.h, RenderThemeQt.h, IDBCursor.h. I added language about that. (I should make the style checker verify that as well at some point.) Committed as http://trac.webkit.org/changeset/69837

Note You need to log in before you can comment on or make changes to this bug.