Bug 32137

Summary: [Website] Style guide should allow indentation of _nested_ namespaces
Product: WebKit Reporter: Jens Alfke <jens>
Component: WebKit WebsiteAssignee: Jens Alfke <jens>
Status: RESOLVED FIXED    
Severity: Minor CC: cjerdonek, eric, hamaji, levin, webkit.review.bot
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch 2 levin: review+, levin: commit-queue-

Description Jens Alfke 2009-12-03 16:00:01 PST
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.
Comment 1 Jens Alfke 2009-12-03 16:10:30 PST
Created attachment 44276 [details]
patch
Comment 2 WebKit Review Bot 2009-12-03 16:11:56 PST
style-queue ran check-webkit-style on attachment 44276 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-03 17:12:24 PST
We'll have to fix check-webkit-style too if we're changing the guidelines.
Comment 4 David Levin 2009-12-03 17:19:47 PST
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.
Comment 5 Chris Jerdonek 2009-12-03 20:21:52 PST
(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.
Comment 6 Jens Alfke 2009-12-04 09:20:51 PST
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.
Comment 7 David Levin 2009-12-04 09:30:24 PST
"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.
Comment 8 Jens Alfke 2009-12-04 09:34:40 PST
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."
Comment 9 David Levin 2009-12-04 12:18:31 PST
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."
Comment 10 Jens Alfke 2009-12-04 13:19:19 PST
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.
Comment 11 Jens Alfke 2009-12-04 13:24:44 PST
Created attachment 44329 [details]
patch 2
Comment 12 WebKit Review Bot 2009-12-04 13:27:34 PST
style-queue ran check-webkit-style on attachment 44329 [details] without any errors.
Comment 13 David Levin 2009-12-17 15:24:38 PST
Comment on attachment 44329 [details]
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).
Comment 14 Eric Seidel (no email) 2010-01-06 23:39:30 PST
Hmm.  Looks like this one got stalled at commit.
Comment 15 Jens Alfke 2010-02-02 15:18:33 PST
Sorry this fell on the floor ... committed r54258.