Patchwork [4/4] Fix leading spaces.

login
register
mail settings
Submitter Ondrej Bilka
Date June 16, 2013, 10:50 a.m.
Message ID <20130616105050.GB4043@domone.kolej.mff.cuni.cz>
Download mbox | patch
Permalink /patch/251668/
State New
Headers show

Comments

Ondrej Bilka - June 16, 2013, 10:50 a.m.
On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote:
> 2013/6/14 Joseph S. Myers <joseph@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
> > joseph@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
Chung-Ju Wu - June 18, 2013, 6:56 a.m.
2013/6/16 Ondřej Bílka <neleai@seznam.cz>:
> On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote:
>> 2013/6/14 Joseph S. Myers <joseph@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
>> > joseph@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
Ondrej Bilka - July 4, 2013, 5:15 p.m.
Ping
On Tue, Jun 18, 2013 at 02:56:52PM +0800, Chung-Ju Wu wrote:
> 2013/6/16 Ondřej Bílka <neleai@seznam.cz>:
> > On Sat, Jun 15, 2013 at 05:13:31PM +0800, Chung-Ju Wu wrote:
> >> 2013/6/14 Joseph S. Myers <joseph@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
> >> > joseph@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

Patch

diff --git a/gcc/.indent.on b/gcc/.indent.on
new file mode 100644
index 0000000..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 0000000..48cdce8
--- /dev/null
+++ b/gcc/c-family/.indent.on
@@ -0,0 +1 @@ 
+placeholder