You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by alanconway <gi...@git.apache.org> on 2018/11/27 21:16:37 UTC
[GitHub] qpid-proton pull request #169: Clang format
GitHub user alanconway opened a pull request:
https://github.com/apache/qpid-proton/pull/169
Clang format
Format command: clang-format -i $(git ls-files -- *.cpp *.hpp *.c *.h)
Style file is /.clang-format, with the following contents:
Language: Cpp # also applies to C
BasedOnStyle: LLVM
IndentWidth: 4
AllowShortIfStatementsOnASingleLine: true
AllowShortLoopsOnASingleLine: true
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alanconway/qpid-proton clang-format
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton/pull/169.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #169
----
commit 5f82739467ed4a936f4755b123f7a4108e273a91
Author: Alan Conway <ac...@...>
Date: 2018-11-27T21:08:40Z
WIP: Support for automating clang-format
/.clang-format - style file for C and C++
/scripts/git-clang-format - reformat git changes only
commit 358a30144fa5124619fb1670ebe2587456e4fb0b
Author: Alan Conway <ac...@...>
Date: 2018-11-27T21:10:56Z
WIP: reformat all c/c++ sources using clang-format.
Format command: clang-format -i $(git ls-files -- *.cpp *.hpp *.c *.h)
Style file is /.clang-format, with the following contents:
Language: Cpp # also applies to C
BasedOnStyle: LLVM
IndentWidth: 4
AllowShortIfStatementsOnASingleLine: true
AllowShortLoopsOnASingleLine: true
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton issue #169: Clang format
Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the issue:
https://github.com/apache/qpid-proton/pull/169
On Wed, Nov 28, 2018 at 12:29 PM Andrew Stitcher <no...@github.com>
wrote:
> *I 100% support having a consistent automated format for the proton
> codebase(s)*
>
> However , I don't think this is quite it (IMO):
>
Have a bash: `clang-format -style=X -dump-style` will give you all the
gory details, also check the clang site for the list of pre-defined styles
(Mozilla and others), you may find one more to your liking.
>
> - I think we should just leave the proton-c codebase with an indent of
> 2 - A lot of changes for marginal gain there.
> - We can have a 4 space indent for the C++ code, I don't think
> having slightly different settings is a problem if the format is
> automatically applied.
>
> Easy to do, just put separate .clang-format files in /c and /cpp . I'd
prefer a common style for both however. The first big reformat will be a
pain (mitigated by automated reformatting for outstanding commits) but the
pain will pass.
If we use different styles for C and C++, should C++ files in the C tests
tree be formated like C or like the C++ binding? The latter will make
automated reformatting more complicated.
>
> - I think we can allow wider lines - it looks like it is wrapping at
> 80 chars, I think 100 would be fine (conservative even) for modern screens.
>
> +1, I went with minimal changes from LLVM but 100 is fine, I find 80 a
bit tight.
>
> - I'm not happy with the reformatting of the header files:
> - Especially reformatting the 2nd and subsequent parameter at a
> variable indent - not cool for variable width fonts!.
>
> No space-based alignment method works for variable width fonts. Are you
proposing we move to tab-based indentation, or to a style that never aligns
anything with the previous line? I'm pretty sure clang-format can do that,
just not sure if that's what you mean.
>
> -
> - Seems to have reformatted blah* x(); to blah *x(); which is fine
> for C code, but not for C++ code. Another area where the setting should
> probably be different.
>
> Depends who you ask, lots of C++ style guides use blah *x; as it more
accurately reflects the precendence of *.
For example `blah *x, *y` means what it looks like; `blah* x, y` does not
(y is a blah, not a blah*)
If you want this I believe the clang-format variable is `PointerAlignment:
Left`. Again I'd prefer we be consistent in C and C++.
> I think the mess of the header files is actually the most important issue
> given that this is user visible documentation. It should look as nice as we
> can make it. Consistency is important, but it shouldn't be ugly to view - I
> currently opine that the reformat has made the header files ugly.
>
I went with adopting an existing style guide with minimal changes, but no
objection if you can find or craft a more beautiful one.
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/qpid-proton/pull/169#issuecomment-442533870>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AHa6XvLNnwh7UBpAiU29hDQB71oGvpGKks5uzsgGgaJpZM4Y2Wqk>
> .
>
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton issue #169: Clang format
Posted by dnwe <gi...@git.apache.org>.
Github user dnwe commented on the issue:
https://github.com/apache/qpid-proton/pull/169
When I've applied clang-format to existing projects, I knocked up a bit of shell script to iterate over the repos with a matrix of various BasedOnStyle and some of the override params and recording the git diff -w --shortstat each time. I then chose whichever config settings caused the smallest diffstat delta going forward.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton issue #169: Clang format
Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/169
__I 100% support having a consistent automated format for the proton codebase(s)__
However , I don't think this is quite it (IMO):
- I think we should just leave the proton-c codebase with an indent of 2 - A lot of changes for marginal gain there.
- We can have a 4 space indent for the C++ code, I don't think having slightly different settings is a problem if the format is automatically applied.
- I think we can allow wider lines - it looks like it is wrapping at 80 chars, I think 100 would be fine (conservative even) for modern screens.
- I'm not happy with the reformatting of the header files:
- Especially reformatting the 2nd and subsequent parameter at a variable indent - not cool for variable width fonts!.
- Seems to have reformatted blah* x(); to blah *x(); which is fine for C code, but not for C++ code. Another area where the setting should probably be different.
- Connected to the first sub point: if the function name and first parameter is too long wrapping both:
blah *pn_xxxx(pn_param1 *x);
To
blah *
pn_xxxx(pn_param1 *x);
I think the mess of the header files is actually the most important issue given that this is user visible documentation. It should look as nice as we can make it. Consistency is important, but it shouldn't be ugly to view - I currently opine that the reformat has made the header files ugly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org