[PING] [PATCH 4/4] Fix leading spaces.
Ping On Tue, Jun 18, 2013 at 02:56:52PM +0800, Chung-Ju Wu wrote: 2013/6/16 Ondřej Bílka nel...@seznam.cz: On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote: 2013/6/14 Joseph S. Myers jos...@codesourcery.com: On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com By having a look at Ondřej's patch, it is nice to fix existing codes with proper whitespace/tab rules. But it covers too much under whole gcc source tree. Like Joseph said, we may accidentally change the cases that need bad whitespace. Perhaps we should gradually fix them? i.e. We can only fix leading spaces for some obvious cases (gcc/*.c gcc/c-family/*.c ...), leaving other source (gcc/testsuite/* gcc/config/* ...) untouched. I uploaded new version that touches only gcc/*.[ch] gcc/c-family/*.[ch] to http://kam.mff.cuni.cz/~ondra/stylepp.tar.bz2 patches are also updated. IMHO, the preliminary whitelist could be: gcc/*.[ch] gcc/c/*.[ch] gcc/c-family/*.[ch] gcc/common/*.[ch] gcc/cp/*.[ch] Anyway what in gcc/config/*.c depends on leading/trailing spaces? In general, I agree with you that all stuff under gcc/config/ and gcc/common/config/ are supposed to follow whitespace rules. But I think it would be better to have corresponding port maintainers make the decision. Your tool and patches look great to me. It helps fixup the existing codes with strong whitespace convention. But I am sorry that I don't have right to approve it. You will need some reviewers to review the patch and give the OK. If you do not receive any response to the patches within two weeks or so, you can 'ping' it with a follow-up mail to remind reviewers. :) Best regards, jasonwucj -- boss forgot system password
Re: [PATCH 4/4] Fix leading spaces.
2013/6/16 Ondřej Bílka nel...@seznam.cz: On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote: 2013/6/14 Joseph S. Myers jos...@codesourcery.com: On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com By having a look at Ondřej's patch, it is nice to fix existing codes with proper whitespace/tab rules. But it covers too much under whole gcc source tree. Like Joseph said, we may accidentally change the cases that need bad whitespace. Perhaps we should gradually fix them? i.e. We can only fix leading spaces for some obvious cases (gcc/*.c gcc/c-family/*.c ...), leaving other source (gcc/testsuite/* gcc/config/* ...) untouched. I uploaded new version that touches only gcc/*.[ch] gcc/c-family/*.[ch] to http://kam.mff.cuni.cz/~ondra/stylepp.tar.bz2 patches are also updated. IMHO, the preliminary whitelist could be: gcc/*.[ch] gcc/c/*.[ch] gcc/c-family/*.[ch] gcc/common/*.[ch] gcc/cp/*.[ch] Anyway what in gcc/config/*.c depends on leading/trailing spaces? In general, I agree with you that all stuff under gcc/config/ and gcc/common/config/ are supposed to follow whitespace rules. But I think it would be better to have corresponding port maintainers make the decision. Your tool and patches look great to me. It helps fixup the existing codes with strong whitespace convention. But I am sorry that I don't have right to approve it. You will need some reviewers to review the patch and give the OK. If you do not receive any response to the patches within two weeks or so, you can 'ping' it with a follow-up mail to remind reviewers. :) Best regards, jasonwucj
Re: [PATCH 4/4] Fix leading spaces.
On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote: 2013/6/14 Joseph S. Myers jos...@codesourcery.com: On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com By having a look at Ondřej's patch, it is nice to fix existing codes with proper whitespace/tab rules. But it covers too much under whole gcc source tree. Like Joseph said, we may accidentally change the cases that need bad whitespace. Perhaps we should gradually fix them? i.e. We can only fix leading spaces for some obvious cases (gcc/*.c gcc/c-family/*.c ...), leaving other source (gcc/testsuite/* gcc/config/* ...) untouched. I uploaded new version that touches only gcc/*.[ch] gcc/c-family/*.[ch] to http://kam.mff.cuni.cz/~ondra/stylepp.tar.bz2 patches are also updated. Anyway what in gcc/config/*.c depends on leading/trailing spaces? To enable which directories it can touch use patch like following one. --- gcc/.indent.on |1 + gcc/c-family/.indent.on |1 + 2 files changed, 2 insertions(+) create mode 100644 gcc/.indent.on create mode 100644 gcc/c-family/.indent.on diff --git a/gcc/.indent.on b/gcc/.indent.on new file mode 100644 index 000..48cdce8 --- /dev/null +++ b/gcc/.indent.on @@ -0,0 +1 @@ +placeholder diff --git a/gcc/c-family/.indent.on b/gcc/c-family/.indent.on new file mode 100644 index 000..48cdce8 --- /dev/null +++ b/gcc/c-family/.indent.on @@ -0,0 +1 @@ +placeholder -- 1.7.10.4
Re: [PATCH 4/4] Fix leading spaces.
2013/6/14 Joseph S. Myers jos...@codesourcery.com: On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com By having a look at Ondřej's patch, it is nice to fix existing codes with proper whitespace/tab rules. But it covers too much under whole gcc source tree. Like Joseph said, we may accidentally change the cases that need bad whitespace. Perhaps we should gradually fix them? i.e. We can only fix leading spaces for some obvious cases (gcc/*.c gcc/c-family/*.c ...), leaving other source (gcc/testsuite/* gcc/config/* ...) untouched. Once Ondřej or others figure out the purpose of those non-obvious cases, they can propose another patches to fix them. Best regards, jasonwucj
Re: [PATCH 4/4] Fix leading spaces.
On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote: 2013/6/14 Joseph S. Myers jos...@codesourcery.com: On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com By having a look at Ondřej's patch, it is nice to fix existing codes with proper whitespace/tab rules. But it covers too much under whole gcc source tree. Like Joseph said, we may accidentally change the cases that need bad whitespace. Perhaps we should gradually fix them? i.e. We can only fix leading spaces for some obvious cases (gcc/*.c gcc/c-family/*.c ...), leaving other source (gcc/testsuite/* gcc/config/* ...) untouched. Here we need a way to clearly mark where preserving whitespaces is important and where it is not. You could add files named say .indent-on .indent-off to enable or disable formating on per directory basis. I leave decision if its whitelist or blacklist upto you. I will follow convention that in files you can selectively disable formatter by comments: /*INDENT-OFF*/ /*INDENT-ON*/ I now accept only files with .c and .h extensions. If you want to format ada/fortran files it is also possible. Once Ondřej or others figure out the purpose of those non-obvious cases, they can propose another patches to fix them. Best regards, jasonwucj
Re: [PATCH 4/4] Fix leading spaces.
On Wed, Jun 12, 2013 at 10:08 PM, Ondřej Bílka nel...@seznam.cz wrote: A followup to previous patch is more general pass that changes leading spaces to tabs followed by at most 8 spaces. http://kam.mff.cuni.cz/~ondra/0004-Formatted-by-leading_space.patch Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. Either by adjusting the committed content or by rejecting the commit(?) Richard.
Re: [PATCH 4/4] Fix leading spaces.
On Thu, Jun 13, 2013 at 10:08:23AM +0200, Richard Biener wrote: On Wed, Jun 12, 2013 at 10:08 PM, Ondřej Bílka nel...@seznam.cz wrote: A followup to previous patch is more general pass that changes leading spaces to tabs followed by at most 8 spaces. http://kam.mff.cuni.cz/~ondra/0004-Formatted-by-leading_space.patch Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. Either by adjusting the committed content or by rejecting the commit(?) It was my next logical step to propose this. For hooks you need a clean source tree or they will pick unrelated code that happens to be in same file. I had hook ready so I copied it to http://kam.mff.cuni.cz/~ondra/stylepp.tar.bz2 To use it, add DIRECTORY/space_pre_commit to your pre-commit hook. I have other checkes that are bit more controversional (for example space after comma which could trigger reformating of subsequent lines.) For it I could add post commit hook that prints: Stylepp found several problems with formating. See git diff (stylepp.patch file if you prefer) Ondra
Re: [PATCH 4/4] Fix leading spaces.
On Thu, 13 Jun 2013, Richard Biener wrote: Btw, rather than these kind of patches I'd appreciate if someone would look at a simple pre(post?)-commit hook that enforces those whitespace rules. In the cpp testsuite we definitely want tests with bad whitespace (e.g. gcc.dg/cpp/backslash*.c) and generally I think it's wrong to be changing whitespace in the testsuite without an understanding of what the test is testing and how the whitespace is irrelevant to that (more generally, cleanups of compiler tests are suspect without such an understanding of what is or is not significant in a particular test). And so you need to allow addition of otherwise bad whitespace there. It's not obvious whether there might be other cases needing such whitespace as well. Either by adjusting the committed content or by rejecting the commit(?) I don't think hooks adjusting committed content are likely to work at all. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 4/4] Fix leading spaces.
A followup to previous patch is more general pass that changes leading spaces to tabs followed by at most 8 spaces. http://kam.mff.cuni.cz/~ondra/0004-Formatted-by-leading_space.patch
