NEW247314
Code style does not match comments given in review related to newlines and preprocessor conditionals 
https://bugs.webkit.org/show_bug.cgi?id=247314
Summary Code style does not match comments given in review related to newlines and pr...
Kimmo Kinnunen
Reported 2022-11-01 05:17:13 PDT
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
Comment 1 2022-11-01 05:17:25 PDT
<rdar://problem/101801755>
Kimmo Kinnunen
Comment 2 2022-11-01 05:30:42 PDT
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
Comment 3 2022-11-01 08:40:37 PDT
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
Comment 4 2022-11-01 09:07:42 PDT
I agree with Ryosuke on this.
Note You need to log in before you can comment on or make changes to this bug.