-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change .clang-format to apply linebreaks before open-braces #1423
base: master
Are you sure you want to change the base?
Conversation
AfterExternBlock: true | ||
BeforeCatch: true | ||
BeforeElse: true | ||
IndentBraces: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting is to control whether the brace itself follows indentation, which i don't think anyone wants.
@@ -80,10 +80,11 @@ IncludeCategories: | |||
IncludeIsMainRegex: '(Test)?$' | |||
IncludeIsMainSourceRegex: '' | |||
IndentCaseLabels: false | |||
IndentGotoLabels: true | |||
IndentGotoLabels: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goto labels are important to be able to quickly identify. Having them left-aligned in the text helps provide that visual indication.
IndentPPDirectives: None | ||
IndentWidth: 2 | ||
IndentWrappedFunctionNames: false | ||
IndentExternBlock: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many headers are wrapped with extern "C"
blocks, don't indent the code found inside those.
extern int LLVMFuzzerTestOneInput(const uint8_t *Data, | ||
size_t Size) { // rfc5769check | ||
extern int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) | ||
{ // rfc5769check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of parameters previously on two lines being able to be on one line. Helps with readability of the function.
@@ -976,22 +1166,28 @@ | |||
* \param pnOutSize: size of char string | |||
* \return | |||
*/ | |||
wchar_t *_ATW(__in char *pszInBuf, __in int nInSize, __out wchar_t **pszOutBuf, __out int *pnOutSize) { | |||
if (!pszInBuf || !pszOutBuf || !pnOutSize || nInSize <= 0) { | |||
wchar_t *_ATW(__in char *pszInBuf, __in int nInSize, __out wchar_t **pszOutBuf, __out int *pnOutSize) |
Check warning
Code scanning / PREfast
Returning uninitialized memory '*pszOutBuf'. A successful path through the function does not set the named Out parameter. Warning
@@ -976,22 +1166,28 @@ | |||
* \param pnOutSize: size of char string | |||
* \return | |||
*/ | |||
wchar_t *_ATW(__in char *pszInBuf, __in int nInSize, __out wchar_t **pszOutBuf, __out int *pnOutSize) { | |||
if (!pszInBuf || !pszOutBuf || !pnOutSize || nInSize <= 0) { | |||
wchar_t *_ATW(__in char *pszInBuf, __in int nInSize, __out wchar_t **pszOutBuf, __out int *pnOutSize) |
Check warning
Code scanning / PREfast
Returning uninitialized memory '*pnOutSize'. A successful path through the function does not set the named Out parameter. Warning
@@ -1005,21 +1201,27 @@ | |||
* \param pnOutSize: size of wchar string | |||
* \return | |||
*/ | |||
char *_WTA(__in wchar_t *pszInBuf, __in int nInSize, __out char **pszOutBuf, __out int *pnOutSize) { | |||
if (!pszInBuf || !pszOutBuf || !pnOutSize || nInSize <= 0) { | |||
char *_WTA(__in wchar_t *pszInBuf, __in int nInSize, __out char **pszOutBuf, __out int *pnOutSize) |
Check warning
Code scanning / PREfast
Returning uninitialized memory '*pszOutBuf'. A successful path through the function does not set the named Out parameter. Warning
@@ -1005,21 +1201,27 @@ | |||
* \param pnOutSize: size of wchar string | |||
* \return | |||
*/ | |||
char *_WTA(__in wchar_t *pszInBuf, __in int nInSize, __out char **pszOutBuf, __out int *pnOutSize) { | |||
if (!pszInBuf || !pszOutBuf || !pnOutSize || nInSize <= 0) { | |||
char *_WTA(__in wchar_t *pszInBuf, __in int nInSize, __out char **pszOutBuf, __out int *pnOutSize) |
Check warning
Code scanning / PREfast
Returning uninitialized memory '*pnOutSize'. A successful path through the function does not set the named Out parameter. Warning
strncpy(fn + fnlen, config_file, fnsz - fnlen); | ||
} | ||
} | ||
} | ||
fn[fnsz] = 0; | ||
if (strstr(fn, "//") != fn) { | ||
if (strstr(fn, "//") != fn) |
Check warning
Code scanning / PREfast
'fn' could be '0': this does not adhere to the specification for the function 'strstr'. See line 1398 for an earlier location where this can occur Warning
@@ -34,9 +34,11 @@ | |||
|
|||
/////////////// io handlers /////////////////// | |||
|
|||
static void udp_server_input_handler(evutil_socket_t fd, short what, void *arg) { | |||
static void udp_server_input_handler(evutil_socket_t fd, short what, void *arg) |
Check warning
Code scanning / PREfast
Function uses '65592' bytes of stack. Consider moving some data to heap. Warning
a243d40
to
8009ead
Compare
8009ead
to
60616fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with any format as long as it is automated :)
Would love to hear an opinion from @ggarber
38a5fdc
to
2b3f0e9
Compare
2b3f0e9
to
bb2d26a
Compare
Hi @jonesmz, thx for looking at this. I personally like more "your" style than the existing coturn style but most of these things are arguable and a matter of personal taste. That's why we decided last year to use a "standard" clang-format (the llvm) that maintains the existing style used in coturn for years. My suggestion would be to keep it and not use a new custom clang-format. Otherwise tomorrow other person can come with similar reasons to change it back. Also we have 28 Pull Requests open right now so any change will force all those authors to reformat their PRs :( |
You don't have a standard clang format. You have a very custom one. A standard clang format would be a single line saying |
Changing .clang-format to
and applying this patch
gives a diff of 31186 lines of changes. |
Ultimately what matters is that the code is readable. While I agree that it's a matter of personal taste, I've yet to find someone who thinks braces-on-same-line is easier to read. Both of the only two maintainers (as far as I can tell???) think this is an improvement to readability. I also don't represent only myself, my team at work also prefers the braces-on-next-line style, and we're trying to get serious about driving improvements to coturn. We had a deadlock in our turnserver service in December 2023 that got CTO level attention and I don't want that happening again. The easier it is for my team members to work with the coturn codebase, the more contributions we can submit. While ultimately we're always going to be bottlenecked by maintainer's ability to review and merge changes, it's a smoother experience with a formatting style that doesn't make my eyes bleed.
I doubt they'll be successful, since you seem to like braces-on-next-line more. But an automated formatting system enables a change of that nature to be quick and easy to try out and make decisions on. The goal is readability and productivity.
This can be done by the maintainer who submits the PR, github has the ability to notate when a PR was modified by the author, or it can be done as a separate fixup commit. Alternatively, a github action that triggers on every submit to Regardless, And I doubt many of the currently open PRs can be merged without rebase today regardless, since my changes to address the github I'd also like to point out that I opened this PR on Feb 1st, and didn't really get any feedback until last week. Of the 29 currently open PRs, 3 of them are dated 2022, 10 of them are dated from 2023. and of the remaining (29-13)=16 PRs that were opened in 2024, 10 of these 16 from 2024 are from myself or my team ( @redraincatching and @WHYHD ). My team has no problem re-running clang-format to comply with the project style, but we do need to have an understanding of what level of feedback to expect so that we can match your pace. I currently have a very large amount of time in the late night / early morning where I can't go anywhere because I'm caring for my newborn, so between bottles and diapers I have nothing else to really do and I'm bored. As a result, I've opened 17 PRs that were merged by @eakraly in the last two weeks. is that too many too quickly? |
This pull request changes the .clang-format file so that a newline is inserted before opening braces in the code.
This changes, for example
to
This has multiple benefits.