RESOLVED FIXED 47646
Add guideline for constructors doing implicit type conversion to coding style. 
https://bugs.webkit.org/show_bug.cgi?id=47646
Summary Add guideline for constructors doing implicit type conversion to coding style.
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-
DetailsFormatted DiffDiff
Proposed fix. (1.44 KB, patch)
2010-10-14 02:55 PDT, David Levin 		darin: review+
levin: commit-queue-
DetailsFormatted DiffDiff
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.