Refactor: Put function return types on their own lines in signatures #157
Conversation
|
Can one of the project admins check and authorise this run please: https://ci.kronosnet.org/job/booth/job/booth-pipeline/job/PR-157/1/input |
|
@clumens Can you review this when you get a chance? Low priority The copyrights need to be updated too, but I'm not sure how we want to do that. We haven't updated most of them in several years, and the copyright "owners" are all over the place. For example, Jiaju was the original committer, but there were several later committers besides. |
There was a problem hiding this comment.
Kind of conditional NACK by me - I can fully agree having return type on their own line is more readable and I like this style too. But this change makes super hard to backport patches, makes git blame much harder and it is questionable if this change is really worth for quite old project such as booth. No matter what, if you think it is really worth the possible troubles, go ahead.
Looks like you've already solved the asciidoc problems (was in notify email, but I cannot find them now), right?
I appreciate your pointing this out :) Speaking only for myself (as the apparent future maintainer of Booth), this tradeoff doesn't bother me much. Pacemaker is much larger than Booth, and we make formatting changes like this one regularly in Pacemaker. It does make backports a bit harder, and it makes I view this as the first step in making Booth code easier for me to read and search, reducing the "cognitive load" of maintaining it. I'm already unfamiliar with the structure and with Unix network/socket programming beyond a very basic level. So I'll take whatever help I can give myself. I'd like to apply the same development guidelines to Booth that we strive for in Pacemaker.
Yes, that was embarrassing... Somehow I had an |
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
5f9fad7 to
84b6079
Compare
|
Can one of the project admins check and authorise this run please: https://ci.kronosnet.org/job/booth/job/booth-pipeline/job/PR-157/2/input |
src/transport.c
Outdated
|
|
||
| static int return_0_booth_site(struct booth_site *v __attribute((unused))) | ||
| static int | ||
| return_0_booth_site(struct booth_site *v __attribute((unused))) |
There was a problem hiding this comment.
This should be __attribute__ for consistency with everywhere else.
I didn't realize gcc would even accept this syntax, and looking into that led me to https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html which says that __attribute__ is the old and busted syntax, and double brackets are the way to go. Of course, we don't do anything the modern way. I just thought it was interesting.
There was a problem hiding this comment.
From the doc you linked:
More specifically, this [double bracket] syntax was first introduced in the C++11 language standard (see Language Standards Supported by GCC)
We support C99 and later, so we can't do this the modern way.
Add C++11 style attribute syntax[29] using double square brackets [[]].
https://en.wikipedia.org/wiki/C23_(C_standard_revision)#Syntax
I agree on making it harder to backport patches, and more importantly to finish off all those cleanups from poki (which I definitely think are worth continuing to merge in). It's still possible of course, it'll just take more manual intervention. Speaking as the outgoing maintainer of booth and the one who won't have to do this manual intervention, I'm fine with this PR if that's what you want to do. |
I just combed through all of those cleanups from poki and tried a cherry-pick or two. Considering the amount of merge conflicts there are already, I intend to go through each commit and apply it manually. Cherry-picking actually feels more difficult than that. So the effect of this PR on those commits doesn't bother me. |
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
84b6079 to
c9d2de4
Compare
This aligns with the Pacemaker Development guide and makes it easier to locate a function's definition. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
These are intermediate files that are generated during man page creation. They may be left behind in the event of an xmllint error, for example. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
c9d2de4 to
ffa20ba
Compare
|
Updated to address review |
3 participants
No description provided.