32137 2009-12-03 16:00:01 -0800 [Website] Style guide should allow indentation of _nested_ namespaces 2010-02-02 15:18:33 -0800 1 1 1 Unclassified WebKit WebKit Website 528+ (Nightly build) All All RESOLVED FIXED P3 Minor --- 1 jens jens cjerdonek eric hamaji levin webkit.review.bot oldest_to_newest 168584 0 jens 2009-12-03 16:00:01 -0800 At http://webkit.org/coding/coding-style.html, rule #3 says "Code inside a namespace should not be indented". This is obviously because most source files have a namespace wrapped around them and indenting the whole file is pointless. However, this shouldn't apply to _nested_ namespace declarations, which are a lot shorter, and by normal C/C++ rules should be indented to distinguish their scope. An example from a patch I'm currently getting reviewed: // HTTPHeaderMap.h namespace WebCore { class HTTPHeaderMap : public HashMap<AtomicString, String, CaseFoldingHash> { ... ... }; namespace HTTPHeaders { extern const char* const Accept; extern const char* const Authorization; ... extern const char* const UserAgent; } } This would look strange if the contents of HTTPHeaders weren't indented. Especially since if HTTPHeaders were made a class instead (which I almost decided to do) the style guide _would_ say to indent it. namespace HTTPHeaders { extern const char* const Accept; extern const char* const Authorization; ... extern const char* const UserAgent; } So I propose to edit the rule to read: 3. Code inside a namespace that takes up the entire file (apart from its #includes) should not be indented. However, if there are nested namespaces, their contents should be indented normally. 168590 1 44276 jens 2009-12-03 16:10:30 -0800 Created attachment 44276 patch 168591 2 webkit.review.bot 2009-12-03 16:11:56 -0800 style-queue ran check-webkit-style on attachment 44276 without any errors. 168599 3 eric 2009-12-03 17:12:24 -0800 We'll have to fix check-webkit-style too if we're changing the guidelines. 168601 4 levin 2009-12-03 17:19:47 -0800 I'll work on a fix fir check-webkit-style. This issue is a bit rare so we won't get a lot of cases with issues anyway. 168628 5 cjerdonek 2009-12-03 20:21:52 -0800 (In reply to comment #1) > Created an attachment (id=44276) [details] > patch > -<li>Code inside a namespace should not be indented. > +<li>Code inside a namespace that takes up the entire file (apart from its > +#includes) should not be indented. However, if there are nested namespaces, > +their contents should be indented normally. Note that there are non-nested namespaces that don't take up the entire file. For example-- namespace JSC { ... } namespace WTF { ... } So I think you should use a word like "outermost". Also, I think consecutive declarations should be treated as a single namespace for the purposes of this rule, rather than as a nesting. For example-- namespace JSC { namespace WREC { (This actually occurs in the code base.) I mentioned these points in an e-mail to the group last month: https://lists.webkit.org/pipermail/webkit-dev/2009-November/010521.html Thanks. 168792 6 jens 2009-12-04 09:20:51 -0800 How does this sound: "When one or more <code>namespace</code> blocks together take up the entire file (apart from its #includes), their contents should not be indented. However, if there are nested namespaces that occupy only a portion of the file, their contents should be indented normally." Thumbs-up and I'll roll a new patch. 168798 7 levin 2009-12-04 09:30:24 -0800 "take up the entire file" seems mildly subjective and it needs multiple exceptions: comments, #include's, #ifdef, #endif, #define, using (see JavaScriptCore/wtf/RefCounted.h) I think outermost namespace is a simpler concept and the right idea. Consecutive namespaces is the right thing for the other idea. 168800 8 jens 2009-12-04 09:34:40 -0800 Well, I was considering the possibility of files that are mostly non-namespaced but have a short namespace in them; in that case it would make sense to indent the namespace. But maybe that doesn't appear anywhere in WebKit. How about: "The contents of an outermost <code>namespace</code> block should not be indented. However, if there are nested namespaces that occupy only a portion of the file, their contents should be indented normally." 168871 9 levin 2009-12-04 12:18:31 -0800 How about this? "The contents of an outermost <code>namespace</code> (and any nested namespaces with the same scope) block should not be indented. The contents of other namespaces should be indented." 168902 10 jens 2009-12-04 13:19:19 -0800 It wasn't intuitively clear to me what "with the same scope" means, but I think this is about as clear as we're going to be able to get it :) I'll update the patch. 168903 11 44329 jens 2009-12-04 13:24:44 -0800 Created attachment 44329 patch 2 168905 12 webkit.review.bot 2009-12-04 13:27:34 -0800 style-queue ran check-webkit-style on attachment 44329 without any errors. 172969 13 44329 levin 2009-12-17 15:24:38 -0800 Comment on attachment 44329 patch 2 Missing ChangeLog. Please add one and feel free to submit (since this issue has already been discussed on WebKit-dev in the past). 178032 14 eric 2010-01-06 23:39:30 -0800 Hmm. Looks like this one got stalled at commit. 187174 15 jens 2010-02-02 15:18:33 -0800 Sorry this fell on the floor ... committed r54258. 44276 2009-12-03 16:10:30 -0800 2009-12-04 13:24:44 -0800 patch textmate stdin rFw9eP.txt text/plain 857 jens ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sIGIvV2ViS2l0 U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKaW5kZXggNmJlMGFlNi4uYjg3YzEzYiAxMDA2 NDQKLS0tIGEvV2ViS2l0U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKKysrIGIvV2ViS2l0 U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKQEAgLTQ1LDcgKzQ1LDkgQEAgaW50IG1haW4o KQogfQogPC9wcmU+CiA8L2xpPgotPGxpPkNvZGUgaW5zaWRlIGEgbmFtZXNwYWNlIHNob3VsZCBu b3QgYmUgaW5kZW50ZWQuCis8bGk+Q29kZSBpbnNpZGUgYSBuYW1lc3BhY2UgdGhhdCB0YWtlcyB1 cCB0aGUgZW50aXJlIGZpbGUgKGFwYXJ0IGZyb20gaXRzCisjaW5jbHVkZXMpIHNob3VsZCBub3Qg YmUgaW5kZW50ZWQuIEhvd2V2ZXIsIGlmIHRoZXJlIGFyZSBuZXN0ZWQgbmFtZXNwYWNlcywKK3Ro ZWlyIGNvbnRlbnRzIHNob3VsZCBiZSBpbmRlbnRlZCBub3JtYWxseS4KIDxoNCBjbGFzcz0icmln aHQiPlJpZ2h0OjwvaDQ+CiA8cHJlIGNsYXNzPSJjb2RlIj4KIC8vIERvY3VtZW50LmgKQEAgLTU2 LDYgKzU4LDEwIEBAIGNsYXNzIERvY3VtZW50IHsKICAgICAuLi4KIH07CiAKK25hbWVzcGFjZSBO ZXN0ZWROYW1lc3BhY2UgeworICAgIC4uLgorfQorCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCiAK IC8vIERvY3VtZW50LmNwcApAQCAtNzksNiArODUsMTAgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewog ICAgICAgICAuLi4KICAgICB9OwogCisgICAgbmFtZXNwYWNlIE5lc3RlZE5hbWVzcGFjZSB7Cisg ICAgLi4uCisgICAgfQorCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCiAKIC8vIERvY3VtZW50LmNw cAo= 44329 2009-12-04 13:24:44 -0800 2009-12-17 15:24:37 -0800 patch 2 textmate stdin VrDu7H.txt text/plain 856 jens ZGlmZiAtLWdpdCBhL1dlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sIGIvV2ViS2l0 U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKaW5kZXggNmJlMGFlNi4uODRmNTkxYyAxMDA2 NDQKLS0tIGEvV2ViS2l0U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKKysrIGIvV2ViS2l0 U2l0ZS9jb2RpbmcvY29kaW5nLXN0eWxlLmh0bWwKQEAgLTQ1LDcgKzQ1LDggQEAgaW50IG1haW4o KQogfQogPC9wcmU+CiA8L2xpPgotPGxpPkNvZGUgaW5zaWRlIGEgbmFtZXNwYWNlIHNob3VsZCBu b3QgYmUgaW5kZW50ZWQuCis8bGk+VGhlIGNvbnRlbnRzIG9mIGFuIG91dGVybW9zdCA8Y29kZT5u YW1lc3BhY2U8L2NvZGU+IGJsb2NrIChhbmQgYW55IG5lc3RlZCBuYW1lc3BhY2VzIHdpdGggdGhl IHNhbWUgc2NvcGUpCitzaG91bGQgbm90IGJlIGluZGVudGVkLiBUaGUgY29udGVudHMgb2Ygb3Ro ZXIgbmVzdGVkIG5hbWVzcGFjZXMgc2hvdWxkIGJlIGluZGVudGVkLgogPGg0IGNsYXNzPSJyaWdo dCI+UmlnaHQ6PC9oND4KIDxwcmUgY2xhc3M9ImNvZGUiPgogLy8gRG9jdW1lbnQuaApAQCAtNTYs NiArNTcsMTAgQEAgY2xhc3MgRG9jdW1lbnQgewogICAgIC4uLgogfTsKIAorbmFtZXNwYWNlIE5l c3RlZE5hbWVzcGFjZSB7CisgICAgLi4uCit9CisKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUKIAog Ly8gRG9jdW1lbnQuY3BwCkBAIC03OSw2ICs4NCwxMCBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAg ICAgICAgIC4uLgogICAgIH07CiAKKyAgICBuYW1lc3BhY2UgTmVzdGVkTmFtZXNwYWNlIHsKKyAg ICAuLi4KKyAgICB9CisKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUKIAogLy8gRG9jdW1lbnQuY3Bw Cg==