47646 2010-10-13 20:06:12 -0700 Add guideline for constructors doing implicit type conversion to coding style. 2010-10-14 22:18:22 -0700 1 1 1 Unclassified WebKit WebKit Website 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 levin levin mjs oldest_to_newest 293915 0 levin 2010-10-13 20:06:12 -0700 The text is meant to follow the consensus from https://lists.webkit.org/pipermail/webkit-dev/2010-September/014523.html 293921 1 70703 levin 2010-10-13 20:22:33 -0700 Created attachment 70703 Proposed fix. 293923 2 70703 abarth 2010-10-13 20:30:33 -0700 Comment on attachment 70703 Proposed fix. Looks good to me. We might wait to see if Darin has comments. 293937 3 mjs 2010-10-13 21:00:06 -0700 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. 294028 4 70703 levin 2010-10-14 02:42:53 -0700 Comment on attachment 70703 Proposed fix. Marking r- per mjs comment. 294032 5 70722 levin 2010-10-14 02:55:06 -0700 Created attachment 70722 Proposed fix. 294453 6 70722 darin 2010-10-14 16:31:32 -0700 Comment on attachment 70722 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. 294580 7 levin 2010-10-14 22:18:13 -0700 (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 70703 2010-10-13 20:22:33 -0700 2010-10-14 02:55:06 -0700 Proposed fix. explicit.diff text/plain 1443 levin ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvQ2hhbmdlTG9nIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK aW5kZXggZjU1YTU4Zi4uMWZmNTk4ZCAxMDA2NDQKLS0tIGEvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMiBAQAorMjAxMC0wOS0zMCAg RGF2aWQgTGV2aW4gIDxsZXZpbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIGluZm9ybWF0aW9uIGFib3V0IGV4cGxpY2l0 IGNvbnN0cnVjdG9ycyB0byBjb2Rpbmcgc3R5bGUuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NzY0NgorCisgICAgICAgICogY29kaW5nL2NvZGluZy1z dHlsZS5odG1sOgorCiAyMDEwLTEwLTAxICBBbGV4IE1hdGhld3MgIDxwb3NzZXNzZWRwZW5ndWlu Ym9iQGdtYWlsLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBGcmFzZXIuCmRpZmYg LS1naXQgYS9XZWJLaXRTaXRlL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbCBiL1dlYktpdFNpdGUv Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sCmluZGV4IDVhZmQ3ODAuLmFmYWE2ZmUgMTAwNjQ0Ci0t LSBhL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCisrKyBiL1dlYktpdFNpdGUv Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sCkBAIC05MzksNiArOTM5LDM1IEBAIHVzaW5nIG5hbWVz cGFjZSBzdGQ7CiAKIDwvb2w+CiAKKzxoMz5DbGFzc2VzPC9oMz4KKworPG9sPgorPGxpPgorVXNl IGV4cGxpY2l0IGZvciBzaW5nbGUgYXJndW1lbnQgY29uc3RydWN0b3JzIHdoZW4gdGhlIGFyZ3Vt ZW50IGlzIG5vdCByZWFzb25hYmx5IHRob3VnaHQgb2YgYXMgYSB0eXBlIGNvbnZlcnNpb24uIElm IGl0IG1heSBiZSB0aG91Z2h0IG9mIGFzIGEgdHlwZSBjb252ZXJzaW9uLCB0aGVuIGltcGxpY2l0 IGNvbnZlcnNpb24gaXMgb2suIElmIHRoZSB0eXBlIGNvbnZlcnNpb24gaXMgY29zdGx5LCBhdm9p ZCBhIGNvbnN0cnVjdG9yIGFsdG9nZXRoZXIgYW5kIG1ha2UgaXQgYSBuYW1lZCBmdW5jdGlvbi4K KworPGg0IGNsYXNzPSJyaWdodCI+UmlnaHQ6PC9oND4KKzxwcmUgY2xhc3M9ImNvZGUiPgorY2xh c3MgTGFyZ2VJbnQgeworcHVibGljOgorICAgIExhcmdlSW50KGludCk7CisuLi4KKworY2xhc3Mg VmVjdG9yIHsKK3B1YmxpYzoKKyAgICBleHBsaWNpdCBWZWN0b3IoaW50IHNpemUpOworLi4uCis8 L3ByZT4KKworPGg0IGNsYXNzPSJ3cm9uZyI+V3Jvbmc6PC9oND4KKzxwcmUgY2xhc3M9ImNvZGUi PgorY2xhc3MgVGFzayB7CitwdWJsaWM6CisgICAgVGFzayhTY3JpcHRFeGVjdXRpb25Db250ZXh0 Kik7CisuLi4KKzwvcHJlPgorPC9saT4KKzwvb2w+CisKIDw/cGhwCiAgICAgaW5jbHVkZSgiLi4v Zm9vdGVyLmluYyIpOwogPz4K 70722 2010-10-14 02:55:06 -0700 2010-10-14 16:31:32 -0700 Proposed fix. explicit.diff text/plain 1474 levin ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvQ2hhbmdlTG9nIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK aW5kZXggZjU1YTU4Zi4uMTNjYmYwZiAxMDA2NDQKLS0tIGEvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0U2l0ZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMiBAQAorMjAxMC0wOS0zMCAg RGF2aWQgTGV2aW4gIDxsZXZpbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIGd1aWRlbGluZSBmb3IgY29uc3RydWN0b3Jz IGRvaW5nIGltcGxpY2l0IHR5cGUgY29udmVyc2lvbiB0byBjb2Rpbmcgc3R5bGUuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NzY0NgorCisgICAgICAg ICogY29kaW5nL2NvZGluZy1zdHlsZS5odG1sOgorCiAyMDEwLTEwLTAxICBBbGV4IE1hdGhld3Mg IDxwb3NzZXNzZWRwZW5ndWluYm9iQGdtYWlsLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBT aW1vbiBGcmFzZXIuCmRpZmYgLS1naXQgYS9XZWJLaXRTaXRlL2NvZGluZy9jb2Rpbmctc3R5bGUu aHRtbCBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCmluZGV4IDVhZmQ3ODAu LjJhMzAyY2MgMTAwNjQ0Ci0tLSBhL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1s CisrKyBiL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCkBAIC05MzksNiArOTM5 LDM2IEBAIHVzaW5nIG5hbWVzcGFjZSBzdGQ7CiAKIDwvb2w+CiAKKzxoMz5DbGFzc2VzPC9oMz4K KworPG9sPgorPGxpPgorVXNlIGEgY29uc3RydWN0b3IgdG8gZG8gYW4gaW1wbGljaXQgY29udmVy c2lvbiB3aGVuIHRoZSBhcmd1bWVudCBpcyByZWFzb25hYmx5IHRob3VnaHQgb2YgYXMgYSB0eXBl IGNvbnZlcnNpb24gYW5kIHRoZSB0eXBlIGNvbnZlcnNpb24gaXMgZmFzdC4gT3RoZXJ3aXNlLCB1 c2UgdGhlIGV4cGxpY2l0IGtleXdvcmQgb3IgYSBmdW5jdGlvbiByZXR1cm5pbmcgdGhlIHR5cGUu Cis8aDQgY2xhc3M9InJpZ2h0Ij5SaWdodDo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+CitjbGFz cyBMYXJnZUludCB7CitwdWJsaWM6CisgICAgTGFyZ2VJbnQoaW50KTsKKy4uLgorCitjbGFzcyBW ZWN0b3IgeworcHVibGljOgorICAgIGV4cGxpY2l0IFZlY3RvcihpbnQgc2l6ZSk7IC8vIE5vdCBh IHR5cGUgY29udmVyc2lvbi4KKyAgICBQYXNzT3duUHRyPFZlY3Rvcj4gY3JlYXRlKEFycmF5KTsg Ly8gQ29zdGx5IGNvbnZlcnNpb24uCisuLi4KKworPC9wcmU+CisKKzxoNCBjbGFzcz0id3Jvbmci Pldyb25nOjwvaDQ+Cis8cHJlIGNsYXNzPSJjb2RlIj4KK2NsYXNzIFRhc2sgeworcHVibGljOgor ICAgIFRhc2soU2NyaXB0RXhlY3V0aW9uQ29udGV4dCopOworLi4uCis8L3ByZT4KKzwvbGk+Cis8 L29sPgorCiA8P3BocAogICAgIGluY2x1ZGUoIi4uL2Zvb3Rlci5pbmMiKTsKID8+Cg==