194930 2019-02-21 19:51:36 -0800 Codify the naming convention for fooIfExists 2019-03-01 11:22:33 -0800 1 1 1 Unclassified WebKit WebKit Website WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa darin dbates fujii ggaren jond mjs webkit-bug-importer zalan oldest_to_newest 1508845 0 rniwa 2019-02-21 19:51:36 -0800 See https://lists.webkit.org/pipermail/webkit-dev/2013-June/025056.html Better late than never. 1508846 1 362688 rniwa 2019-02-21 19:53:35 -0800 Created attachment 362688 Updates the style guideline 1508852 2 362688 darin 2019-02-21 20:03:07 -0800 Comment on attachment 362688 Updates the style guideline Looks fine 1508853 3 rniwa 2019-02-21 20:04:28 -0800 I'd wait a day or two to give a chance for people to respond on webkit-dev in the case they don't like this convention since the discussion happened 5 years ago. 1508855 4 362688 zalan 2019-02-21 20:14:50 -0800 Comment on attachment 362688 Updates the style guideline View in context: https://bugs.webkit.org/attachment.cgi?id=362688&action=review > Websites/webkit.org/ChangeLog:8 > + Updating the coding style guidleline after the discussion following guideline 1508885 5 362688 dbates 2019-02-21 21:29:59 -0800 Comment on attachment 362688 Updates the style guideline View in context: https://bugs.webkit.org/attachment.cgi?id=362688&action=review > Websites/webkit.org/code-style.md:686 > +```cpp > +StyleResolver* styleResolverIfExists(); > +StyleResolver& styleResolver(); > +``` First, if I were to vote today (02/21/2019), I would vote for StyleResolver& styleResolver() and StyleResolver* existingStyleResolver(), which was what Darin suggested when he wrote "I can’t remember what we decided on for our convention. I personally prefix the thing()/existingThing() pairing, but maybe I was outvoted?" in bug #194764, comment 18. Since Darin r+'ed this patch I guess he changed his mind. Funny thing though, if he didn't change his mind then his vote and mine would tip the scale towards: StyleResolver& styleResolver() and StyleResolver* existingStyleResolver() Funny, huh? I don't know how you came to think StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver() are the "right" choice, but Darin r+'ed it. So, I guess he warmed up to styleResolverIfExists() even though he started that webkit-dev thread you mentioned with "Lets bike shed! For some time, functions with names like **fooIfExists** and ensureFoo have been bothering me." I went through every single email in that thread (was a different time back made it clear what their position was with a "+1"). Here's what I found and it disagrees with your choices and I'm only mentioning it for completeness since Darin r+'ed the patch (and again he was the one that was initially unhappy). I did skip a few emails that just gave some other naming suggestions that nobody ever replied to as liking: Emil A Eklund eae at chromium.org: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025057.html> (BTW she no longer contributes) +1 StyleResolver* optionalStyleResolver() and StyleResolver& requiredStyleResolver() But suggested StyleResolver* optionalStyleResolver() and StyleResolver& styleResolver() (So to me this implies these are +1 by her as well) Geoffrey Garen ggaren at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025076.html> "I love this!" to Emil A Eklund's suggestion of StyleResolver* optionalStyleResolver() and StyleResolver& styleResolver() Ryosuke Niwa (You): <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025058.html> Suggest requireStyleResolver() Darin Adler: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025060.html> "Tentative +1" = 0.5 to Ryosuke's suggestion of requireStyleResolver(): "I’m warming to this idea. Maybe we can use “require” as a term of art, analogous to the way we use “create”, to mean “create if not already created”" Simon Fraser: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025061.html> -1 for requireStyleResolver(): "requireStyleResolver() sounds like it would return a bool." Maciej Stachowiak: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025061.html> Sounds like not in favor of requireStyleResolver(). Suggests to drop qualifier. Balazs Kelemen kbalazs at webkit.org: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025072.html> (BTW no longer contributes) Suggests styleResolverIfExists() and styleResolver() Timothy Hatcher timothy at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025073.html>: Suggests existingStyleResolver() and styleResolver(). Dan Bernstein mitz at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025073.html> "I like it." to Timothy Hatcher's suggestion. Filip Pizlo fpizlo at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025119.html> Likes styleResolverIfExists(): "That's why I like "fooIfExists"" Andreas Kling akling at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025074.html> (BTW no longer contributes) "+1 to these two." to Timothy Hatcher's suggestion of existingStyleResolver() and styleResolver(). Elliott Sprehn esprehn at chromium.org: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025080.html> (BTW no longer contributes) -1 to existingStyleResolver() and styleResolver(): "This doesn't make sense since calling styleResolver() again won't create a new one so it's also "existing style resolver"." +1 to foo() and ensureFoo() Maciej Stachowiak mjs at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025083.html> Disagrees with Elliott's suggestion. Suggests "fooIfExists/foo. But I agree with others on the thread that optionalFoo or existingFoo might be better names, particularly given std::optional" Gavin Barraclough barraclough at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-June/025070.html> -1 requireStyleResolver: "I find ‘requireStyleResolver()’ a little confusing." Suggests acquireStyleResolver(). My composite breakdown of the responses: Name for "guaranteed to give you a resolver" function: StyleResolver& styleResolver() => +8 (Emil A Eklund, Geoffrey Garen, Balazs Kelemen, Timothy Hatcher, Dan Bernstein, Andreas Kling, Elliott Sprehn, Maciej Stachowiak) StyleResolver& requireStyleResolver() => +1.5 = +1 "Ryosuke Niwa" + 0.5 "Darin Adler" Name for function that returns, but does not create a resolver: StyleResolver* styleResolverIfExists() => +3 (Balazs Kelemen, Filip Pizlo, Maciej Stachowiak) StyleResolver* existingStyleResolver() => +3 (Timothy Hatcher, Dan Bernstein, Andreas Kling) StyleResolver* optionalStyleResolver() => +2 (Emil A Eklund, Geoffrey Garen) Adding in my votes and focusing on the winning results: StyleResolver& styleResolver() => +9 (Emil A Eklund, Geoffrey Garen, Balazs Kelemen, Timothy Hatcher, Dan Bernstein, Andreas Kling, Elliott Sprehn, Maciej Stachowiak, Daniel Bates) StyleResolver* existingStyleResolver() => +4 (Timothy Hatcher, Dan Bernstein, Andreas Kling, Daniel Bates) Adding in Darin's votes if he changes his mind back to what he wrote in bug #194764, comment 18, final score is: StyleResolver& styleResolver() => +10 (Emil A Eklund, Geoffrey Garen, Balazs Kelemen, Timothy Hatcher, Dan Bernstein, Andreas Kling, Elliott Sprehn, Maciej Stachowiak, Daniel Bates, Darin Adler) StyleResolver* existingStyleResolver() => +5 (Timothy Hatcher, Dan Bernstein, Andreas Kling, Daniel Bates, Darin Adler) 1508886 6 dbates 2019-02-21 21:35:12 -0800 Darn. I missed some replies that spilled over to July: <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025122.html> 1508889 7 dbates 2019-02-21 21:43:56 -0800 Ryosuke Niwa (You): <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025122.html> +1 Balazs Kelemen's StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver(): "Maybe StyleResolver* styleResolverIfExists() StyleResolver& styleResolver()" Brady Eidson: <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025123.html> +1 StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver() Dan Bernstein mitz at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025124.html> +1 StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver() Maciej Stachowiak mjs at apple.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025130.html> +1 StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver() Bem Jones-Bey bjonesbe at adobe.com: <https://lists.webkit.org/pipermail/webkit-dev/2013-July/025132.html> (BTW no longer contributes) +1 StyleResolver* styleResolverIfExists() and StyleResolver& styleResolver() So, final tally for styleResolver() and styleResolverIfExists(), not including me or Darin: StyleResolver& styleResolver() => +10 (Emil A Eklund, Geoffrey Garen, Balazs Kelemen, Timothy Hatcher, Dan Bernstein, Andreas Kling, Elliott Sprehn, Maciej Stachowiak, Brady Eidson, Bem Jones-Bey) StyleResolver* styleResolverIfExists() => +7 (Balazs Kelemen, Filip Pizlo, Maciej Stachowiak, Ryosuke Niwa, Dan Bernstein, Brady Eidson, Bem Jones-Bey) So, even if I voted and Darin changed his mind on styleResolverIfExists(), styleResolverIfExists() would win. Okay. I'm fine with that. 1508892 8 rniwa 2019-02-21 22:14:18 -0800 Yeah, it was a pretty lengthy debate and there were many suggestions and up-votes. It was easier for me to go through because I had an email thread in my email archive. Thanks for summarizing the results though. 1511476 9 rniwa 2019-03-01 11:21:17 -0800 Committed r242273: <https://trac.webkit.org/changeset/242273> 1511478 10 webkit-bug-importer 2019-03-01 11:22:33 -0800 <rdar://problem/48516506> 362688 2019-02-21 19:53:35 -0800 2019-02-21 20:03:07 -0800 Updates the style guideline bug-194930-20190221195335.patch text/plain 2020 rniwa SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiAyNDE5MjgpCisrKyBXZWJzaXRlcy93 ZWJraXQub3JnL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE5 LTAyLTIxICBSeW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIENvZGlm eSB0aGUgbmFtaW5nIGNvbnZlbnRpb24gZm9yIGZvb0lmRXhpc3RzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTQ5MzAKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVcGRhdGluZyB0aGUgY29kaW5nIHN0eWxl IGd1aWRsZWxpbmUgYWZ0ZXIgdGhlIGRpc2N1c3Npb24gZm9sbG93aW5nCisgICAgICAgIGh0dHBz Oi8vbGlzdHMud2Via2l0Lm9yZy9waXBlcm1haWwvd2Via2l0LWRldi8yMDEzLUp1bmUvMDI1MDU2 Lmh0bWwKKworICAgICAgICAqIGNvZGUtc3R5bGUubWQ6CisKIDIwMTktMDItMjAgIEpvbiBEYXZp cyAgPGpvbmRAYXBwbGUuY29tPgogCiAgICAgICAgIEFkZGVkIG1hY09TIE1vamF2ZSBidWlsZHMg dG8gdGhlIFdlYktpdCBCdWlsZCBBcmNoaXZlcy4KSW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcv Y29kZS1zdHlsZS5tZAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJzaXRlcy93ZWJraXQub3JnL2NvZGUtc3R5 bGUubWQJKHJldmlzaW9uIDI0MTkyOCkKKysrIFdlYnNpdGVzL3dlYmtpdC5vcmcvY29kZS1zdHls ZS5tZAkod29ya2luZyBjb3B5KQpAQCAtNjc2LDYgKzY3NiwzNCBAQCBib29sIGNvbnZlcnRUb0FT Q0lJKHNob3J0Kiwgc2l6ZV90KTsKIGJvb2wgdG9BU0NJSShzaG9ydCosIHNpemVfdCk7CiBgYGAK IAorW10oI25hbWVzLWlmLWV4aXN0cykgVGhlIGdldHRlciBmdW5jdGlvbiBmb3IgYSBtZW1iZXIg dmFyaWFibGUgc2hvdWxkIG5vdCBoYXZlIGFueSBzdWZmaXggb3IgcHJlZml4IGluZGljYXRpbmcg dGhlIGZ1bmN0aW9uIGNhbiBvcHRpb25hbGx5IGNyZWF0ZSBvciBpbml0aWFsaXplIHRoZSBtZW1i ZXIgdmFyaWFibGUuIFN1ZmZpeCB0aGUgZ2V0dGVyIGZ1bmN0aW9uIHdoaWNoIGRvZXMgbm90IGF1 dG9tYXRpY2FsbHkgY3JlYXRlIHRoZSBvYmplY3Qgd2l0aCBgSWZFeGlzdHNgIGlmIHRoZXJlIGlz IGEgdmFyaWFudCB3aGljaCBkb2VzLgorCisjIyMjIyMgUmlnaHQ6CisKK2BgYGNwcAorU3R5bGVS ZXNvbHZlciogc3R5bGVSZXNvbHZlcklmRXhpc3RzKCk7CitTdHlsZVJlc29sdmVyJiBzdHlsZVJl c29sdmVyKCk7CitgYGAKKworIyMjIyMjIFdyb25nOgorCitgYGBjcHAKK1N0eWxlUmVzb2x2ZXIq IHN0eWxlUmVzb2x2ZXIoKTsKK1N0eWxlUmVzb2x2ZXImIGVuc3VyZVN0eWxlUmVzb2x2ZXIoKTsK K2BgYAorCisjIyMjIyMgUmlnaHQ6CisKK2BgYGNwcAorRnJhbWUqIGZyYW1lKCk7CitgYGAKKwor IyMjIyMjIFdyb25nOgorCitgYGBjcHAKK0ZyYW1lKiBmcmFtZUlmRXhpc3RzKCk7CitgYGAKKwog W10oI25hbWVzLXZhcmlhYmxlLW5hbWUtaW4tZnVuY3Rpb24tZGVjbCkgTGVhdmUgbWVhbmluZ2xl c3MgdmFyaWFibGUgbmFtZXMgb3V0IG9mIGZ1bmN0aW9uIGRlY2xhcmF0aW9ucy4gQSBnb29kIHJ1 bGUgb2YgdGh1bWIgaXMgaWYgdGhlIHBhcmFtZXRlciB0eXBlIG5hbWUgY29udGFpbnMgdGhlIHBh cmFtZXRlciBuYW1lICh3aXRob3V0IHRyYWlsaW5nIG51bWJlcnMgb3IgcGx1cmFsaXphdGlvbiks IHRoZW4gdGhlIHBhcmFtZXRlciBuYW1lIGlzbid0IG5lZWRlZC4gVXN1YWxseSwgdGhlcmUgc2hv dWxkIGJlIGEgcGFyYW1ldGVyIG5hbWUgZm9yIGJvb2xzLCBzdHJpbmdzLCBhbmQgbnVtZXJpY2Fs IHR5cGVzLgogCiAjIyMjIyMgUmlnaHQ6Cg==