Adapt fixcc.py to use Astyle and/or emacs

Adapt fixcc.py to use Astyle instead of emacs,
  results in patch set 3

and then try the resulting cleaned-up fixcc.py, with emacs,
  results in patch set 4

Finally, we seem happiest with fixcc.py using Astyle,
  results in patch set 5

[Keith, 2011-07-03 02:36:32]

This script, applied to the .cc .icc and .hh files in flower/ and lily/, gives a
clean make check.

http://codereview.appspot.com/4662074/diff/7001/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (right):

http://codereview.appspot.com/4662074/diff/7001/scripts/auxiliar/fixcc.py#new...
scripts/auxiliar/fixcc.py:79: # space after `operator'
Overloaded operators get a space?  "operator + () {"

[Carl, 2011-07-03 02:41:15]

Can you make it possible for us to see the diff caused by applying this script
to the files you've mentioned?

Thanks,

Carl

[Graham Percival (old account), 2011-07-03 07:22:07]

On 2011/07/03 02:41:15, Carl wrote:
> Can you make it possible for us to see the diff caused by applying this script
> to the files you've mentioned?

I'll make a separate git branch showing the output once a few minor issues are
done.


Keith:
1. all // comments are now hard-left (i.e. in column 0).  Is that intentional? 
it looks weird in the diff.

2. some multi-line comments are still modofied:
diff --git a/lily/break-alignment-interface.cc b/lily/break-alignment-interface.
index d368f83..0f08880 100644
--- a/lily/break-alignment-interface.cc
+++ b/lily/break-alignment-interface.cc
@@ -65,7 +65,7 @@ Break_alignment_interface::ordered_elements (Grob *grob)
 
   vector<Grob *> writable_elts (elts);
   /*
-    Copy in order specified in BREAK-ALIGN-ORDER.
+   Copy in order specified in BREAK-ALIGN-ORDER.
   */
   vector<Grob *> new_elts;
   for (; scm_is_pair (order); order = scm_cdr (order))


#2 isn't important, but #1 looks like a bug.

Thanks so much for working on this!

Cheers,
- Graham

[Keith, 2011-07-03 08:15:04]

On 2011/07/03 07:22:07, Graham Percival wrote:
> 1. all // comments are now hard-left (i.e. in column 0).

This is fixed in the new patch set.
The pre-filter was collapsing all whitespace, bringing single-line comments to
the left edge, and astyle is conservative about re-indenting these, unless we
change options.

I just made the pre-filter less aggressive, because I hope the prefilter
eventually withers away to nothing.

> 2. some multi-line comments are still modified:

>    vector<Grob *> writable_elts (elts);
>    /*
> -    Copy in order specified in BREAK-ALIGN-ORDER.
> +   Copy in order specified in BREAK-ALIGN-ORDER.
>    */

Astyle preserved the alignment, relative to the /*, in the original.  I see
comments being well-preserved -- re-indented as solid blocks with the contents
intact -- if I compare the results to master.

I'll push this fix to dev/gperciva-astyle, so we don't get confused between
versions

[Keith, 2011-07-04 03:33:19]

I really, really, wish Astyle had a fully gnu-compliant mode out of the box.
I went through the regexp's again and think this is good to go.

I'll push a commit showing this run over the whole repository to 
<http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=shortlog;h=refs/heads/de...>

http://codereview.appspot.com/4662074/diff/4031/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/align-interface.cc#newco...
lily/align-interface.cc:375: "stacking-dir ");
The prefilter in fixcc.py doesn't like the "dangling )"

http://codereview.appspot.com/4662074/diff/4031/lily/audio-item.cc
File lily/audio-item.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/audio-item.cc#newcode43
lily/audio-item.cc:43: channel_ (0)
The indenter in emacs un-indents the member initializers, whether we move the
','  or not, but the names remain aligned

http://codereview.appspot.com/4662074/diff/4031/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/beam-quanting.cc#newcode474
lily/beam-quanting.cc:474: std::priority_queue < Beam_configuration *,
std::vector<Beam_configuration *>,
If the template is broken into two lines, astyle does the angle braces wrongly

http://codereview.appspot.com/4662074/diff/4031/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/beam.cc#newcode70
lily/beam.cc:70: max_connect_ = 1000; // infinity
prefilter in fixcc.py collapses spaces

http://codereview.appspot.com/4662074/diff/4031/lily/context.cc
File lily/context.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/context.cc#newcode283
lily/context.cc:283: add_listener (GET_LISTENER
(new_context->create_context_from_event),
astyle does not (yet) indent line-broken statements if there is no appropriate
operator to align to

http://codereview.appspot.com/4662074/diff/4031/lily/context.cc#newcode330
lily/context.cc:330: accepts);
we can set the limit on how far astyle will indent to align with parentheses; I
put 60 columns as the maximum

http://codereview.appspot.com/4662074/diff/4031/lily/general-scheme.cc
File lily/general-scheme.cc (right):

http://codereview.appspot.com/4662074/diff/4031/lily/general-scheme.cc#newcode55
lily/general-scheme.cc:55: tail = SCM_CDRLOC (*tail);
this is why I need the prefilter in fixcc.py

http://codereview.appspot.com/4662074/diff/4031/lily/general-scheme.cc#newcod...
lily/general-scheme.cc:217: + default_value_string + "'.");
the prefilter in fixcc.py moves the + s

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (right):

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py#new...
scripts/auxiliar/fixcc.py:120: \#[ \t]*define[ \t]+([^\n]*\\\n)*[^\n]*))''',
I made a pattern to skip over defines, as it skips over comments, and leave them
un-touched.  The old method of un-doing the damage prefilter did to #defines,
made me nervous.

http://codereview.appspot.com/4662074/diff/4031/test.c
File test.c (right):

http://codereview.appspot.com/4662074/diff/4031/test.c#newcode1
test.c:1: #	  include	< foo(bar(baz >
compare to Patch set 2 to see the effect of 
fixcc.py with astyle

http://codereview.appspot.com/4662074/diff/4031/testfile_fixcc_emacs.c
File testfile_fixcc_emacs.c (right):

http://codereview.appspot.com/4662074/diff/4031/testfile_fixcc_emacs.c#newcode1
testfile_fixcc_emacs.c:1: /* testfile from fixcc.py, as *output* from fixcc.py
using emacs */
The output of the old, more-picky fixcc+emacs is left unchanged by fixcc+astyle.
(No diff set 2--3)

http://codereview.appspot.com/4662074/diff/4031/testfile_in.c
File testfile_in.c (right):

http://codereview.appspot.com/4662074/diff/4031/testfile_in.c#newcode39
testfile_in.c:39: !OK  OK
These comments were messed-up on input, and they remain so on output.  I just
tested with the testfile; I didn't write it.

[t.daniels_treda.co.uk, 2011-07-04 07:43:05]

<k-ohara5a5a@oco.net> wrote Monday, July 04, 2011 4:33 AM


>I really, really, wish Astyle had a fully gnu-compliant mode out of 
>the
> box.
> I went through the regexp's again and think this is good to go.

I agree.  This is a big improvement, and would
give us control over the layout style ourselves
(rather than "what emacs does").  The odd niggle
is not important; consistency is.

Thanks for pursuing this to a satisfactory
conclusion, Keith!

Trevor 

[t.daniels_treda.co.uk, 2011-07-04 07:43:05]

<k-ohara5a5a@oco.net> wrote Monday, July 04, 2011 4:33 AM


>I really, really, wish Astyle had a fully gnu-compliant mode out of 
>the
> box.
> I went through the regexp's again and think this is good to go.

I agree.  This is a big improvement, and would
give us control over the layout style ourselves
(rather than "what emacs does").  The odd niggle
is not important; consistency is.

Thanks for pursuing this to a satisfactory
conclusion, Keith!

Trevor 

[janneke_gnu.org, 2011-07-04 09:28:26]

Trevor Daniels writes:

> I agree.  This is a big improvement, and would
> give us control over the layout style ourselves
> (rather than "what emacs does").

While the work being done here is possibly a good thing, let me remind
you once more that we are a GNU project and thus use the GNU standards
and thus we need no control over, we need not decide about, we need not
discuss layout style.  That's a feature.

The GNU standards are implemented by Emacs, and if it makes an error,
then that's a serious bug that should be reported (to emacs).  It seems
to me that someone is spending a lot of effort `just' to accomodate
people who haven't found how awesome Emacs is to edit code and thus
introduce layout problems.  This makes it now easier to use another
editor than Emacs, which may or may not be an improvement.  While
choice is good, in this case it decreases the need for non-Emacs
users to try Emacs, and I'm not at all sure if that's a feature.

Greetings,
Jan.

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar®  http://AvatarAcademy.nl

[Graham Percival, 2011-07-04 09:41:56]

On Mon, Jul 04, 2011 at 11:28:19AM +0200, Jan Nieuwenhuizen wrote:
> While the work being done here is possibly a good thing, let me remind
> you once more that we are a GNU project and thus use the GNU standards
> and thus we need no control over, we need not decide about, we need not
> discuss layout style.  That's a feature.

then WTF is fixcc.py ?!  (the original one, pre-astyle)
LilyPond has a *huge* history of deviating from GNU standards.

> This makes it now easier to use another
> editor than Emacs, which may or may not be an improvement.  While
> choice is good, in this case it decreases the need for non-Emacs
> users to try Emacs, and I'm not at all sure if that's a feature.

I'm sorry, I thought this was a project to create aesthetically
beautiful notation.  Not an "emacs is awesome and let's force
everybody to use my favorite text editor" project.

- Graham

[c_sorensen, 2011-07-04 16:26:15]

On 7/4/11 3:28 AM, "Jan Nieuwenhuizen" <janneke@gnu.org> wrote:

> Trevor Daniels writes:
> 
>> I agree.  This is a big improvement, and would
>> give us control over the layout style ourselves
>> (rather than "what emacs does").
> 
> While the work being done here is possibly a good thing, let me remind
> you once more that we are a GNU project and thus use the GNU standards
> and thus we need no control over, we need not decide about, we need not
> discuss layout style.  That's a feature.
> 
> The GNU standards are implemented by Emacs, and if it makes an error,
> then that's a serious bug that should be reported (to emacs).  It seems
> to me that someone is spending a lot of effort `just' to accomodate
> people who haven't found how awesome Emacs is to edit code and thus
> introduce layout problems.  This makes it now easier to use another
> editor than Emacs, which may or may not be an improvement.  While
> choice is good, in this case it decreases the need for non-Emacs
> users to try Emacs, and I'm not at all sure if that's a feature.

I've tried Emacs, and I can't see the benefits justifying fighting through
the learning curve.  It's not comfortable for me; I have 30+ years of
experience with vi(m), so it's hardwired in my bones.

We need more, not fewer, developers on LilyPond.  Why would we want to force
people to try Emacs as a condition of helping out with LilyPond?  Do we want
to try to prohibit people from using Jedit, or Frescobaldi, since they're
not Emacs?  

And I refuse, on principle, to accept a standard that says "do it the way
software package XYZ does it."  This is not a standard, it's an excuse for
not having a standard.  That's behavior I'd expect from a proprietary
software house, not from an organization that advocates software freedom.
It's a peculiar type of freedom that says "you're free to do anything you
want with the software, except make changes in an editor other than our
approved editor."  Now I realize that nobody is forced to follow the GNU
coding standards; they can modify the code to their heart's content and keep
a separate distro from the official distro.  But there's something
philosophically wrong about this, and it seems to me that it's largely
laziness on the part of the FSF.    They are unwilling to define a standard
that is other than machine-readable (a set of elisp macros and settings is
only machine-readable, even if it's human understandable).

So although this is a GNU project, I feel perfectly comfortable with
advocating a style that is enforced by a project-specific tool that can be
used by contributors working in any editor, rather than tying it to Emacs.

Thanks,

Carl

[t.daniels_treda.co.uk, 2011-07-04 16:32:32]

Jan Nieuwenhuizen wrote Monday, July 04, 2011 10:28 AM


> Trevor Daniels writes:
>
>> I agree.  This is a big improvement, and would
>> give us control over the layout style ourselves
>> (rather than "what emacs does").
>
> While the work being done here is possibly a good thing, let me 
> remind
> you once more that we are a GNU project and thus use the GNU 
> standards
> and thus we need no control over, we need not decide about, we 
> need not
> discuss layout style.  That's a feature.

I looked at the GNU coding standards for C:
http://www.gnu.org/prep/standards/standards.html#Formatting

They are defined very loosely, and are recommendations rather than
hard requiements.  To quote:

"We don’t think of these recommendations as requirements, because
it causes no problems for users if two different programs have 
different
formatting styles.

"But whatever style you use, please use it consistently, since a 
mixture
of styles within one program tends to look ugly. If you are 
contributing
changes to an existing program, please follow the style of that 
program."

So what we are doing seems to me to be quite compatible with GNU
standards.

> The GNU standards are implemented by Emacs, and if it makes an 
> error,
> then that's a serious bug that should be reported (to emacs).  It 
> seems
> to me that someone is spending a lot of effort `just' to 
> accomodate
> people who haven't found how awesome Emacs is to edit code and 
> thus
> introduce layout problems.  This makes it now easier to use 
> another
> editor than Emacs, which may or may not be an improvement.  While
> choice is good, in this case it decreases the need for non-Emacs
> users to try Emacs, and I'm not at all sure if that's a feature.

Surely you're not serious!  Are you??  Decreasing the need for Emacs
is the whole point of this discussion.

> Greetings,
> Jan.

Trevor

[Keith, 2011-07-04 20:45:23]

On 2011/07/04 09:28:26, janneke_gnu.org wrote:
> 
> The GNU standards are implemented by Emacs, 

Well, it looks like the problems with fixcc.py were caused by regexps, not
emacs.

Also, I tried to make the output of the modified fixcc+asytle pass unchanged
through emacs, but the very many cases of line-broken asssignments will be
different.
  long_variable_name = first_term
    + second_term; // emacs
  long_variable_name = first_term
                       + second_term; // astyle
Emacs users will forget to run fixcc (because they are lazy heathens, as all vi
users know :-) causing distracting whitespace changes in the diffs.

So *if* we can add back regexps to fixcc.py that do what neither emacs nor
programmers do automatically:
  pad parentheses me->origin ();
  unpad parenthesis-groups if ((a & mask) == (d & mask))
  pad operators   array[i + 1]
without breaking other things, then we could go back to call emacs for the
indenting.

Then hopefully the script would commute with emacs
  fixcc.py (code) == emacs ( fixcc.py (code))
Patches welcome.

But until that is done, my proposal remains this shorter prefilter+astyle.

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (left):

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py#old...
scripts/auxiliar/fixcc.py:148: ('((if|while)\s+\(([^\)]|\([^\)]*\))*\))\s*;',
'\\1\n;'),
This is what was line-breaking the semicolon on an unbraced do-while!
do
 something;
while (relevant)
 ;

So I was wrong to blame emacs for this one when I said
> You'll notice that some of the gnu coding standards
> you linked to, like braces on do-while, are
> specifically designed to defend against emacs's

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (right):

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py#new...
scripts/auxiliar/fixcc.py:64: # trailing operator, but don't un-trail assignment
operators or close angle-braces >
Looking through the existing code, line-broken assignments /do/ get the '=' on
the second line
  long_name
    = long_initializer;
so I could restore that.

[Keith, 2011-07-05 08:00:59]

On 7/4/11 11:32 PM, "Jan Nieuwenhuizen" <janneke <at> gnu.org> wrote:

>> Also, I tried to make the output of the modified fixcc+asytle pass
>> unchanged through emacs, but the very many cases of line-broken
>> asssignments will be different.

>
>> Emacs users will forget to run fixcc
>
> That won't do, sorry.  Let's make it as easy as possible for non-Emacs
> users; but choosing a coding style that Emacs does not support is
> a no-go.

Well, the good news is
+ Most of the problems we had were due to the prefilter, not emacs
+ emacs onlyd does the indentation, so probably we need not worry about which
emacs version
+ emacs can indent spaces instead of tabs
+ The prefilter can protect block comments from emacs indentation

Therefore here is a new patch set showing a restricted pre-filter using emacs. 
I have only had a quick look at the diffs but what I saw looked good.  Runnign
the formatter on the full tree gives a clean make; I have a busy Tuesday, but at
the end of it I can run make check and report back here.

Since the operation is emacs( prefilter (code)), then further editing using
emacs will make no changes -- assuming emacs indentation is idempotent, which is
my big word for the day.

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (right):

http://codereview.appspot.com/4662074/diff/4031/scripts/auxiliar/fixcc.py#new...
scripts/auxiliar/fixcc.py:64: # trailing operator, but don't un-trail assignment
operators or close angle-braces >
On 2011/07/04 20:45:23, Keith wrote:
> Looking through the existing code, line-broken assignments /do/ get the '='
on
> the second line
>   long_name
>     = long_initializer;
> so I could restore that.

Done.

[Graham Percival (old account), 2011-07-06 14:40:56]

wait, WTF just happened?  We're no longer using astyle, and instead we'll be
requiring everybody to install emacs?!

This doesn't sound like a step forward to me...

[janneke, 2011-07-06 16:33:12]

On 2011/07/06 14:40:56, Graham Percival wrote:
> wait, WTF just happened?  We're no longer using astyle, and instead we'll be
> requiring everybody to install emacs?!
> 
> This doesn't sound like a step forward to me...

Yay! And while everyone has it installed, why not give
it a second (or third) try! *evil grin*

[Graham Percival (old account), 2011-07-06 17:26:02]

On 2011/07/06 16:33:12, jan.nieuwenhuizen wrote:
> On 2011/07/06 14:40:56, Graham Percival wrote:
> > wait, WTF just happened?  We're no longer using astyle, and instead we'll be
> > requiring everybody to install emacs?!
> 
> Yay! And while everyone has it installed, why not give
> it a second (or third) try! *evil grin*

... I'm sorry, I think there might be a miscommunication again.

Are you seriously proposing that, instead of asking people to download astyle
2.02 which is 150.8 Kb, we ask people to download emacs, which is 40 megabytes
or something?

I mean, this is a joke, right?  Keith misinterpreted some comment of yours and
changed his patch to use emacs instead of astyle because of a joke?

Please tell me that you are not seriously proposing that we use emacs for
indentation.

[janneke, 2011-07-06 19:04:11]

On 2011/07/06 17:26:02, Graham Percival wrote:

> Please tell me that you are not seriously proposing that we use emacs for
> indentation.

Can't you tell from my evil grin that I'm serious?

I do think that it's important that the suggestion that
Emacs would not indent GNU code correctly has been withdrawn.

It isn't all that black and white.  We've been using fixcc with
emacs on and off and the only ones running it were Han-Wen and
I and we both have emacs installed -- and not astyle probably.

If you look at debian's popularity contest, you'll find that
emacsen have an installed base of about 22%, while astyle
has an installed base of 0.84%.

On the one hand, it is silly in a monty-pythonesque way to
depend on and run Emacs to indent code where astyle would do.
On the other hand, it is equally silly to suggest that astyle
would be required to indent GNU code after editing with Emacs.

[Keith, 2011-07-06 19:30:52]

On 2011/07/06 19:04:11, jan.nieuwenhuizen wrote:
> On 2011/07/06 17:26:02, Graham Percival wrote:
> > Please tell me that you are not seriously proposing that we use 
> > emacs for indentation.

Myself, I propose fixcc(astyle) and give fixcc(emacs) as an alternative.  Now
that it protects block comments, I now think fixcc(emacs) is a viable
alternative; its output passed make check overnight.

Version changes in astyle/emacs concern me less now that I see that 
the unusual bits of gnu style are handled in the prefilter.

However, I can't seem to teach astyle to leave emacs' indentation choices
completely intact.  So if an emacs user runs the indenter over and entire file,
there will be a few distracting changes in the diff.

So we can
1) trust emacs users to control their auto-indenter, or
2) forgive the distracting whitespace changes, or
3) all of us install emacs and let fixcc use it as indenter.
If nobody sees a killer problem with any of these then I suggest the
build-meister may choose by whim or coin-flip.

[janneke, 2011-07-06 19:42:47]

On 2011/07/06 19:30:52, Keith wrote:
> So we can
> 1) trust emacs users to control their auto-indenter, or
> 2) forgive the distracting whitespace changes, or
> 3) all of us install emacs and let fixcc use it as indenter.
> If nobody sees a killer problem with any of these then I suggest the
> build-meister may choose by whim or coin-flip.

4) forgive whitespace changes for now and add an option for
   astyle/fix astyle's gnu indentation

[Graham Percival, 2011-07-06 19:55:46]

On Wed, Jul 06, 2011 at 07:04:11PM +0000, jan.nieuwenhuizen@gmail.com wrote:
> I do think that it's important that the suggestion that
> Emacs would not indent GNU code correctly has been withdrawn.

I agree; I'm not quite certain where that suggestion arose, but
tomorrow I will check the GOP-PROP and make sure it does not
appear there.

> On the one hand, it is silly in a monty-pythonesque way to
> depend on and run Emacs to indent code where astyle would do.
> On the other hand, it is equally silly to suggest that astyle
> would be required to indent GNU code after editing with Emacs.

But we're not talking about GNU code.  We're talking about
lilypond code, and lilypond code has never been formatted exactly
the way that emacs does -- otherwise fixcc.py wouldn't exist!

I don't think it's silly to suggest that we have a small,
editor-netural program to indent code.  A few years ago, we
started using a moderately small, little-used source control
system (i.e. git).  A year or two ago, we started using a very
small, little-used script (git-cl) to begin code reviews.  We use
texi2html for the html docs.  etc.

I really don't see what's wrong with astyle.  I agree that it is
important that we do not slander programs/projects/people, so as I
said I will check the GOP-PROP to make sure it does not imply that
emacs cannot format GNU code.  But in terms of which program we
use to format lilypond code, I see no compelling reason to use
40-meg emacs instead of 150-kay astyle.

[Graham Percival, 2011-07-06 20:03:22]

On Wed, Jul 06, 2011 at 07:30:52PM +0000, k-ohara5a5a@oco.net wrote:
> However, I can't seem to teach astyle to leave emacs' indentation
> choices completely intact.  So if an emacs user runs the indenter over
> and entire file, there will be a few distracting changes in the diff.

... does not compute?  What do you mean "if an emacs user runs the
indenter over an entire file" ?

If fixcc+astyle is adopted, there will be one indenter for
lilypond code.  Namely, fixcc+astyle.  As a vim user, I wouldn't
dream of auto-indenting a .cc file with vim.  As a kate user, I'm
sure that Reinhold wouldn't dream of auto-indenting a .cc file
with kate.  As a microsoft word user, James Lowe wouldn't dream of
auto-indenting a .cc file with word (at least, I expect that he
has to use word at some point for his job).

> 1) trust emacs users to control their auto-indenter, or

This.  If somebody wants to own a rabid rotweiller or hungry tiger
as their pet, that's their business; but if that beast gets loose
on the street, that's a different story.
(yes, most cities have by-laws against owning dangerous animals,
but I'm not most cities)

> If nobody sees a killer problem with any of these then I suggest the
> build-meister may choose by whim or coin-flip.

I don't think I should pick, regardless of whim or not.  I'd like
to reach consensus, or at least have an absolutely clear reason
why we can't reach consensus.  I think that another few days of
discussion -- or perhaps even a full week -- won't hurt.

Cheers,
- Graham

[k-ohara5A5A_oco.net, 2011-07-07 05:06:35]

-------- Original Message --------
> From: "Graham Percival" <graham@percival-music.ca>
> Sent: Wednesday, July 06, 2011 1:03 PM
> To: k-ohara5a5a@oco.net, carl.d.sorensen@gmail.com, 
percival.music.ca@gmail.com, t.daniels@treda.co.uk, janneke@gnu.org, 
c_sorensen@byu.edu, jan.nieuwenhuizen@gmail.com, lilypond-devel@gnu.org, 
reply@codereview.appspotmail.com
> Subject: Re: Adapt fixcc.py to use Astyle and/or emacs (issue4662074)
> 
> On Wed, Jul 06, 2011 at 07:30:52PM +0000, k-ohara5a5a@oco.net wrote:
> > However, I can't seem to teach astyle to leave emacs' indentation
> > choices completely intact.  So if an emacs user runs the indenter over
> > and entire file, there will be a few distracting changes in the diff.
> 
> ... does not compute?  What do you mean "if an emacs user runs the
> indenter over an entire file" ?

I suppose I was thinking the normal way to use emacs's "indent-region" was 
on the whole file before saving.  If people normally just highlight a 
region they are working on and Ctrl-Alt-\ or something, then Viva 
fixcc+astyle !
-
Keith

[Keith, 2011-07-08 05:45:27]

I will be away for a long weekend, back Monday, late late Monday, UTC.

[Keith, 2011-07-14 04:33:57]

It seems everyone is happy to use astyle as the indenter
(it sure is faster) so here is one last patch set showing what this does to the
sample set of C++ files.

Running fixcc(astyle) on the source tree again gives a clean make check.

http://codereview.appspot.com/4662074/diff/26001/Documentation/contributor/pr...
File Documentation/contributor/programming-work.itexi (left):

http://codereview.appspot.com/4662074/diff/26001/Documentation/contributor/pr...
Documentation/contributor/programming-work.itexi:361: macro, the fixcc.py script
will move the final parenthesis up one line
The script itself (not emacs) did this bad deed, and it does no longer.

[t.daniels_treda.co.uk, 2011-07-14 07:48:24]

k-ohara5a5a@oco.net wrote Thursday, July 14, 2011 5:33 AM


> It seems everyone is happy to use astyle as the indenter
> (it sure is faster) so here is one last patch set showing what 
> this does
> to the sample set of C++ files.
>
> Running fixcc(astyle) on the source tree again gives a clean make 
> check.

I was hoping to recommend going with this, so we could
wrap up this issue, but then I found

- new_context->event_source ()->
-   add_listener (GET_LISTENER 
(new_context->create_context_from_event),
+ new_context->event_source ()->
+ add_listener (GET_LISTENER 
(new_context->create_context_from_event),

which looks wrong to me (by any measure of code clarity).

There are examples of this in lily/context.cc

Trevor

[Keith, 2011-07-15 03:51:08]

On Thu, 14 Jul 2011 00:47:58 -0700, Trevor Daniels <t.daniels@treda.co.uk>
wrote:

>
> I was hoping to recommend going with this, so we could
> wrap up this issue, but then I found
>
> - new_context->event_source ()->
> -   add_listener (GET_LISTENER
> (new_context->create_context_from_event),
> + new_context->event_source ()->
> + add_listener (GET_LISTENER
> (new_context->create_context_from_event),
>
> which looks wrong to me (by any measure of code clarity).
>

I commented on this in Patch Set 3, but it doesn't bother me too badly.  I put
in an enhancement request for Astyle to indent multi-line statements.

GNU style, interpreted literally, would put the binary operator -> on the second
line.  I could easily make fixcc.py enforce this, if you think it helps:

new_context->event_source ()
->add_listener (GET_LISTENER (new_context->create_context_from_event),
                 ly_symbol2scm ("CreateContext"));

Emacs by default would indent the continuation lines two columns further.

[t.daniels_treda.co.uk, 2011-07-15 07:47:18]

Keith OHara wrote Friday, July 15, 2011 4:50 AM


> On Thu, 14 Jul 2011 00:47:58 -0700, Trevor Daniels 
> <t.daniels@treda.co.uk> wrote:
>
>>
>> I was hoping to recommend going with this, so we could
>> wrap up this issue, but then I found
>>
>> - new_context->event_source ()->
>> -   add_listener (GET_LISTENER
>> (new_context->create_context_from_event),
>> + new_context->event_source ()->
>> + add_listener (GET_LISTENER
>> (new_context->create_context_from_event),
>>
>> which looks wrong to me (by any measure of code clarity).
>>
>
> I commented on this in Patch Set 3,

Ah, sorry, I didn't follow all the twists and turns in
this carefully enough.

> but it doesn't bother me too badly.

Well, no.  Overall the proposed change is a definite
improvement.

> I put in an enhancement request for Astyle to indent multi-line 
> statements.

Good.  So I would be happy to see your patch applied
as it is now, with this change being implemented whenever
it appears.  I don't see that causing much difficulty with
broken patches.

> GNU style, interpreted literally, would put the binary operator -> 
> on the second line.  I could easily make fixcc.py enforce this, if 
> you think it helps:
>
> new_context->event_source ()
> ->add_listener (GET_LISTENER 
> (new_context->create_context_from_event),
>                 ly_symbol2scm ("CreateContext"));

I'm not sure it does.  It's the indentation that immediately
shows the line is a continuation.

Trevor

[Keith, 2011-07-30 03:16:07]

http://codereview.appspot.com/4662074/diff/26001/scripts/auxiliar/fixcc.py
File scripts/auxiliar/fixcc.py (right):

http://codereview.appspot.com/4662074/diff/26001/scripts/auxiliar/fixcc.py#ne...
scripts/auxiliar/fixcc.py:70: ('
(::|&&|\|\||<=|>=|!=|\|=|==|\+=|-=|\*=|/=|\?|<|\+|-|=|/|:|&XXX|\||\*XXX) *\n(
*)([^\s#])', '\n\\2\\1 \\3'),
The snippet match for #include cuts the string before the #, hiding it from the
regex engine. If we change this to leave the # for the C analysis, we can use a
negative lookahead.

http://codereview.appspot.com/4662074/diff/26001/scripts/auxiliar/fixcc.py#ne...
scripts/auxiliar/fixcc.py:119: \#[ \t]*define[ \t]+([^\n]*\\\n)*[^\n]*))''',
consider leaving the # outside the match, so the CXX regex can find it. This
makes it easier to avoid deleting the newline before a #pragma or #define

http://codereview.appspot.com/4662074/diff/26001/scripts/auxiliar/fixcc.py#ne...
scripts/auxiliar/fixcc.py:138: "([^"\n]|\\")*"))''',
consider leaving the "" outside the <match>, so the CXX regex can see it. 
Otherwise the CXX regex does not know if the pre-matched portion is a string or
a #define or a comment.