Bug 247314
| Summary: | Code style does not match comments given in review related to newlines and preprocessor conditionals | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> |
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | ap, cdumez, darin, rniwa, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | Safari Technology Preview | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
Kimmo Kinnunen
Code style does not match comments given in review related to newlines and preprocessor conditionals
C++ code snippet under review:
#include "IPCTesterProxy.h"
#if ENABLE(IPC_TESTING_API)
namespace WebKit {
Was denied with comment
> Also need a blank line between #if and namespace.
Define what is the rule to follow in this case and codify it somehow in the code style.
This is a currently non-deterministic in current reviews: some people might require, some allow and some deny.
This causes un-needed churn during review, wasting time in review as well as re-authoring, rebasing, re-submitting and re-waiting slow EWS results and patch landings.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/101801755>
Kimmo Kinnunen
One review example in https://github.com/WebKit/WebKit/pull/5952 but possibly there are others.
Ryosuke, would you have a rule suggestion or requirement?
I'm interpreting your rule something like:
[](#linebreaking-mandatory-new-line) A new line should follow each of the lines containing following source elements:
* End of the initial license block comment
* After last consecutive preprocessor statement in preprocessor conditionals `#if`, `#else`, `#elif` and `#endif`
* After last non-preprocessor statement before above mentioned preprocessor statements
* Last line of `.h`, `.cpp`, `.js`, `.html` and `.css` files
Example:
---
#if ENABLE(IPC_TESTING_API)
namespace WebKit {
// ...
}
#endif
namespace WTF {
// ..
}
---
Example (Connection.h):
--
#if USE(UNIX_DOMAIN_SOCKETS)
#include <wtf/unix/UnixFileDescriptor.h>
#endif
#if OS(DARWIN)
#include <mach/mach_port.h>
#include <wtf/MachSendRight.h>
#include <wtf/OSObjectPtr.h>
#include <wtf/spi/darwin/XPCSPI.h>
#endif
#if OS(WINDOWS)
#include <wtf/win/Win32Handle.h>
#endif
#if USE(GLIB)
#include <wtf/glib/GSocketMonitor.h>
#endif
#if ENABLE(IPC_TESTING_API)
#include "MessageObserver.h"
#endif
---
Example wrong in current Connection.h:
---
#if USE(UNIX_DOMAIN_SOCKETS)
using Handle = UnixFileDescriptor;
#elif OS(WINDOWS)
using Handle = Win32Handle;
#elif OS(DARWIN)
using Handle = MachSendRight;
#endif
--
Example current Connection.h should be:
--
#if USE(UNIX_DOMAIN_SOCKETS)
using Handle = UnixFileDescriptor;
#elif OS(WINDOWS)
using Handle = Win32Handle;
#elif OS(DARWIN)
using Handle = MachSendRight;
#endif
--
Or is the comment more of just the first file-global ifdef that comments out the code?
Something like:
[](#linebreaking-mandatory-new-line) A new line should follow each of the lines containing following source elements:
* End of the initial license block comment
* After last preprocessor statement in preprocessor conditionals `#if`, `#else`, `#elif` and `#endif` that begins the enablement of conditionally compiled code in .cpp
* After last non-preprocessor statement before above mentioned preprocessor statements
Ryosuke Niwa
I think this code is correct:
#if USE(UNIX_DOMAIN_SOCKETS)
using Handle = UnixFileDescriptor;
#elif OS(WINDOWS)
using Handle = Win32Handle;
#elif OS(DARWIN)
using Handle = MachSendRight;
#endif
In this case, statements within if-elif is indented, so we don't need to insert a blank line to differentiate / distinguish if-elif-endif further.
Darin Adler
I agree with Ryosuke on this.