Some new coding style rules
Good morning WebKit crowd, I'd like to discuss some style issues, that I'm frequently unsure about: 1. namespace closing brace It was discussed in http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563 , but with no real result. When writing headers, do we need the "// namespace Foo" comment after the namespace closing brace? I think it's visual noise and would like to see a style rule that forbids it. (A rule that says we need this would also be fine with me, as long as we write it down. But I'd vote for removing those comments, I don't trust them anyways, if I'm unsure.) namespace WebCore { ... } // namespace WebCore 2. ENABLE(FOO) #endif comments #if ENABLE(FOO) .. #endif // ENABLE(FOO) Shall we remove the comment, or require it explicitely in the style rules? 3. Inclusion order Say Foo.h is guarded with ENABLE(FOO) macros Foo.h: #if ENABLE(FOO) namespace WebCore { class Foo... } #endif What's the correct inclusion order in Foo.cpp: #include "config.h" #include "Foo.h" #if ENABLE(FOO) .. #endif or #include "config.h" #include "Foo.h" #if ENABLE(FOO) .. #endif (I think the former, as it makes no sense to process the header, if I already know it's enclosed in ENABLE(FOO) blocks) I'd be happy if we could agree on rules and write them down. Cheers, Niko
On Wed, Aug 4, 2010 at 9:29 AM, Nikolas Zimmermann < zimmermann@physik.rwth-aachen.de> wrote:
Good morning WebKit crowd,
I'd like to discuss some style issues, that I'm frequently unsure about:
1. namespace closing brace It was discussed in http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563, but with no real result.
When writing headers, do we need the "// namespace Foo" comment after the namespace closing brace? I think it's visual noise and would like to see a style rule that forbids it. (A rule that says we need this would also be fine with me, as long as we write it down. But I'd vote for removing those comments, I don't trust them anyways, if I'm unsure.)
namespace WebCore { ... } // namespace WebCore
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
3. Inclusion order
Say Foo.h is guarded with ENABLE(FOO) macros Foo.h: #if ENABLE(FOO) namespace WebCore {
class Foo...
} #endif
What's the correct inclusion order in Foo.cpp: #include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
or
#include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
I don't see the difference between these two examples. I'm not sure if this is what you were asking, but here's another: Should we do this: #include "config.h" #include "Foo.h" #if ENABLE(FOO) #include "other" #include "includes" ... #endif or this #include "config.h" #include "Foo.h" #include "other" #include "includes" #if ENABLE(FOO) ... #endif or this #include "config.h" #if ENABLE(FOO) #include "Foo.h" #include "other" #include "includes" ... #endif The last one is definitely required in custom JS* or V8* files since the Foo.h file will only exist if the guard is enabled. But besides this one case where we have to, I feel splitting the #include config from the main header file adds unnecessary noise and is ugly. So, for the common case, I'd like to suggest we go with my first example since it doesn't include a bunch of unnecessary files, but it keeps our standard convention even in (the many) files that are entirely encapsulated in an ENABLE block. I'd agree that it'd be nice to get some of this stuff nailed down so we're consistent.
(I think the former, as it makes no sense to process the header, if I already know it's enclosed in ENABLE(FOO) blocks)
I'd be happy if we could agree on rules and write them down.
Cheers, Niko
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Am 04.08.2010 um 13:15 schrieb Jeremy Orlow:
On Wed, Aug 4, 2010 at 9:29 AM, Nikolas Zimmermann <zimmermann@physik.rwth-aachen.de
wrote: Good morning WebKit crowd,
I'd like to discuss some style issues, that I'm frequently unsure about:
1. namespace closing brace It was discussed in http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563 , but with no real result.
When writing headers, do we need the "// namespace Foo" comment after the namespace closing brace? I think it's visual noise and would like to see a style rule that forbids it. (A rule that says we need this would also be fine with me, as long as we write it down. But I'd vote for removing those comments, I don't trust them anyways, if I'm unsure.)
namespace WebCore { ... } // namespace WebCore
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
3. Inclusion order
Say Foo.h is guarded with ENABLE(FOO) macros Foo.h: #if ENABLE(FOO) namespace WebCore {
class Foo...
} #endif
What's the correct inclusion order in Foo.cpp: #include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
or
#include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
I don't see the difference between these two examples.
Oops, sorry I meant to write: #include "config.h" #include "Foo.h" #if ENABLE(FOO) or #include "config.h" #if ENABLE(FOO) #include "Foo.h" ... sorry for the confusion. Cheers, Niko
On Aug 4, 2010, at 7:15 AM, Jeremy Orlow wrote:
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
I find these comments especially helpful when there are nested #ifs involved. I also find them helpful (though less so) when there are no nested #ifs, but a lot of code is between the #if/#endif. I don't find them useful when a whole file (either .h or .cpp) is compiled out. -Adam
On 04.08.10 20.04, Adam Roben wrote:
On Aug 4, 2010, at 7:15 AM, Jeremy Orlow wrote:
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
I find these comments especially helpful when there are nested #ifs involved. I also find them helpful (though less so) when there are no nested #ifs, but a lot of code is between the #if/#endif. I don't find them useful when a whole file (either .h or .cpp) is compiled out.
I agree, nested #ifs, especially when non-indented, are a lot easier to read with ending comments. Tor Arne
On Aug 4, 2010, at 1:29 AM, Nikolas Zimmermann wrote:
1. namespace closing brace It was discussed in http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563, but with no real result.
When writing headers, do we need the "// namespace Foo" comment after the namespace closing brace? I think it's visual noise and would like to see a style rule that forbids it. (A rule that says we need this would also be fine with me, as long as we write it down. But I'd vote for removing those comments, I don't trust them anyways, if I'm unsure.)
namespace WebCore { ... } // namespace WebCore
My personal preference: We should omit it.
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
My personal preference: We should omit it.
3. Inclusion order
Say Foo.h is guarded with ENABLE(FOO) macros Foo.h: #if ENABLE(FOO) namespace WebCore {
class Foo...
} #endif
What's the correct inclusion order in Foo.cpp: #include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
or
#include "config.h" #include "Foo.h"
#if ENABLE(FOO) .. #endif
(I think the former, as it makes no sense to process the header, if I already know it's enclosed in ENABLE(FOO) blocks)
My personal preference: The latter. One of the main reasons we include "Foo.h" first in "Foo.cpp" is to test that the header has the proper includes to be used standalone. It may be useful to do that test even if the header is supposed to be wrapped entirely in ENABLE(FOO). But this is a close call. I’d be happy either way. -- Darin
On Aug 4, 2010, at 10:43 AM, Darin Adler wrote:
On Aug 4, 2010, at 1:29 AM, Nikolas Zimmermann wrote:
1. namespace closing brace It was discussed in http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563, but with no real result.
When writing headers, do we need the "// namespace Foo" comment after the namespace closing brace? I think it's visual noise and would like to see a style rule that forbids it. (A rule that says we need this would also be fine with me, as long as we write it down. But I'd vote for removing those comments, I don't trust them anyways, if I'm unsure.)
namespace WebCore { ... } // namespace WebCore
My personal preference: We should omit it.
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
Shall we remove the comment, or require it explicitely in the style rules?
My personal preference: We should omit it.
Comments at the end of an #if block can help code readability if the #ifs are nested. - Maciej
On Wed, August 4, 2010 at 11:56:53 PM, Sam Weinig wrote:
On Wed, Aug 4, 2010 at 1:29 AM, Nikolas Zimmermann <zimmermann@physik.rwth-aachen.de> wrote:
namespace WebCore { ... } // namespace WebCore
2. ENABLE(FOO) #endif comments
#if ENABLE(FOO) .. #endif // ENABLE(FOO)
I like these two forms of comments.
As do I. My preference for rules is: - Always use "} // namespace Foo" comments with namespaces. - Always use "#endif // ENABLE(FOO)" comments if there are more than 10-15 lines of code between the matching #if/#else/#elif, or if the #endif is nested. But do not treat it as a warning/error if an #endif macro has a well-formed comment and doesn't obey the 'required' rules in the previous sentence (unless this inconsistency would drive anyone nuts). I like both of these comments for the following reasons: - You don't have to scroll up or otherwise jump around the source to know what the right curly brace or the #endif macro is used for. In the namespace case, the block almost always spans most of the file, so there's rarely a case when it's less than 10 lines away from the left curly brace. Or to put it another way, the "namespace Foo {" line is always near the top of the file, and the "} // namespace Foo" line is always near the bottom of the file, so it's rare that you see them both at the same time. In the case of the #endif macro, I find that the comments are useful if the corresponding #if/#else/#elif is more than 10-15 lines away (e.g., more than a screenful), or if there is any #if macro nesting such that #endif statements can appear next to each other (see Platform.h). - When merging, having these comments are a great help to both the person doing the merge and to merge algorithms. New methods are often added near the end of a namespace, which means the "}" for the end of the method looks the same as the "}" for the end of a namespace (especially with our current namespace indent rules), so merges with conflicts sometimes end up "repurposing" the namespace curly brace for the end of a new method. Providing an explicit comment prevents this behavior, and provides a common anchor point in both the original ("ours") and new ("theirs") files. Likewise, #endif macros (especially when grouped together due to nesting) will frequently get "repurposed" by merge algorithms. This can create more work (especially the way some macros are nested in Platform.h) if there are changes to both files being merged. -- On a related note, I think we should also define rules for indenting #if macros. I think this is orthogonal to using comments, though, and I prefer having comments regardless of the indent rules. Note that macro indenting has already started (haphazardly) in Platform.h and perhaps a few other places. Here are some examples: #if ENABLE(FOO) #if PLATFORM(BAR) #if USE(BAZ) ... #else ... #endif // USE(BAZ) #endif // PLATFORM(BAR) #endif // ENABLE(FOO) #if ENABLE(FOO) # if PLATFORM(BAR) # if USE(BAZ) ... # else ... # endif // USE(BAZ) # endif // PLATFORM(BAR) #endif // ENABLE(FOO) #if ENABLE(FOO) #if PLATFORM(BAR) #if USE(BAZ) ... #else ... #endif // USE(BAZ) #endif // PLATFORM(BAR) #endif // ENABLE(FOO) My original opinion was to never indent macros, but after staring at the above examples, I do think indenting makes them easier to read. I'm still trying to decide whether I like the "#" anchored on the left or whether I prefer it anchored to if/else/elif/endif macro names, though. Dave
participants (8)
-
Adam Roben
-
Darin Adler
-
David Kilzer
-
Jeremy Orlow
-
Maciej Stachowiak
-
Nikolas Zimmermann
-
Sam Weinig
-
Tor Arne Vestbø