[devtools] add clang-format.py #6790

Merged
merged 2 commits into from Oct 23, 2015

Conversation

Projects
None yet
5 participants
@MarcoFalke
Member

MarcoFalke commented Oct 9, 2015

This will add a python convenience wrapper for clang-format. The python code includes a list with tested versions (tested_versions) which can easily be updated in the future.

[ci skip]

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 9, 2015

Member

Not sure if possible: a script that only clang-formats changed/added code (would require some clever git operations) would be nice.

Member

jonasschnelli commented Oct 9, 2015

Not sure if possible: a script that only clang-formats changed/added code (would require some clever git operations) would be nice.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 9, 2015

Member

@jonasschnelli Not sure if clang-format-diff.py is what you are looking for.

Member

MarcoFalke commented Oct 9, 2015

@jonasschnelli Not sure if clang-format-diff.py is what you are looking for.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 10, 2015

Contributor

After looking at the amount of changes clang-format generates and actually looking at such changes and looking at our release process (releasing 0.10, 0.11, ...), I think it doesn't make sense to apply the formatting in one shot for all the source files.
It only has sense for formatting new files and (maybe) for very stable files, like primites/* etc.

And as written above, I do not think the wrapper (in fact, -i and "tested" version check) is needed at all.

Contributor

paveljanik commented Oct 10, 2015

After looking at the amount of changes clang-format generates and actually looking at such changes and looking at our release process (releasing 0.10, 0.11, ...), I think it doesn't make sense to apply the formatting in one shot for all the source files.
It only has sense for formatting new files and (maybe) for very stable files, like primites/* etc.

And as written above, I do not think the wrapper (in fact, -i and "tested" version check) is needed at all.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 11, 2015

Member

@paveljanik I think it doesn't make sense to apply the formatting in one shot for all the source files.

I think we all agree on this.

@paveljanik I do not think the [...] "tested" version check is needed at all.

How would one verify a format PR when different versions produce different output? Also, it may be useful the extend the clang-format.py to skip git subtrees et al. (c.f. sipa@a126129#diff-6ca10bb3f43dbe234b1fce27929ce0d4R10 ).

Member

MarcoFalke commented Oct 11, 2015

@paveljanik I think it doesn't make sense to apply the formatting in one shot for all the source files.

I think we all agree on this.

@paveljanik I do not think the [...] "tested" version check is needed at all.

How would one verify a format PR when different versions produce different output? Also, it may be useful the extend the clang-format.py to skip git subtrees et al. (c.f. sipa@a126129#diff-6ca10bb3f43dbe234b1fce27929ce0d4R10 ).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 13, 2015

Member

Code review ACK, although I'm not sure I see when this should be used.

Needs mention in contrib/devtools/README.md

Member

laanwj commented Oct 13, 2015

Code review ACK, although I'm not sure I see when this should be used.

Needs mention in contrib/devtools/README.md

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 13, 2015

Member
Member

sipa commented Oct 13, 2015

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 13, 2015

Contributor

It should definitely be used when committing new files...

Contributor

paveljanik commented Oct 13, 2015

It should definitely be used when committing new files...

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 16, 2015

Member

@laanwj Done.
@sipa I have tested 3.6.{0,1,2} and they all produce the same output, so I have added them to tested_versions.

Member

MarcoFalke commented Oct 16, 2015

@laanwj Done.
@sipa I have tested 3.6.{0,1,2} and they all produce the same output, so I have added them to tested_versions.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 23, 2015

Member

I wonder whether a solution could be to include a small snippet of representative unformatted code, and have the wrapper script check whether it results in the expected output?

I like this idea a lot.

Member

laanwj commented Oct 23, 2015

I wonder whether a solution could be to include a small snippet of representative unformatted code, and have the wrapper script check whether it results in the expected output?

I like this idea a lot.

@laanwj laanwj merged commit 8c15f33 into bitcoin:master Oct 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 23, 2015

Merge pull request #6790
8c15f33 [trivial] Update contrib/devtools/README.md (MarcoFalke)
338f62f [devtools] add clang-format.py (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-clangFormatWrapper branch Oct 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment