Re: Code formatter

[Graham Percival]

On Fri, Nov 13, 2009 at 04:59:20PM -0000, Trevor Daniels wrote:
>
> Chris Snyder wrote Friday, November 13, 2009 4:35 PM
>> Graham Percival wrote:
>>> I know that you're thinking "this is ridiculous", but unless
>>> somebody does it, newbies will continue to face this difficulty.
>>> This job won't get done by itself.
>>
>> Yes, I do think it's ridiculous. As I understand, you're saying, "go 
>> find a tool that makes the code conform to a standard we haven't 
>> defined yet."
>
> Chris is right: if progress is to be made the rules
> must be defined first.  They can be defined
> incrementally; in fact some already exist.  If we
> can't agree on the rules there is little point in
> searching for a tool.

FFS!    ok, **I** will start doing this investigation.

1) git pull -r; cd lily
2) astyle *.cc
3) git diff > foo.patch
4) ls -lh foo.patch
-rw-r--r-- 1 gperciva gperciva 3.4M 2009-11-13 17:14 foo.patch

hmm, that's not too optimistic.  Let's see...
--- a/lily/accidental-engraver.cc 
+++ b/lily/accidental-engraver.cc 
@@ -29,61 +29,61 @@ 
 class Accidental_entry 
 { 
 public: 
-  bool done_; 
+    bool done_;


hmm, indenting variables to be 4 spaces instead of 2 spaces.
Hmm... ick, a *ton* of things in the patch look like this.  Let's
try making it 2 spaces instead.

5) git checkout *.cc
6) astyle --style=gnu

I couldn't immediately see a "use 2 spaces for indents", but let's
try this "gnu" option!
6) git diff > foo.patch
7) ls -lh foo.patch
-rw-r--r-- 1 gperciva gperciva 2.1M 2009-11-13 17:18 foo.patch

well, it's smaller, at least.

8)  looking at a diff...
--- a/lily/accidental-engraver.cc
+++ b/lily/accidental-engraver.cc
@@ -27,18 +27,18 @@
 #include "translator.icc"

 class Accidental_entry
-{
-public:
-  bool done_;
+  {
+  public:
+    bool done_;

interesting.  is that really the GNU style?  maybe I should check.
Or wait, maybe this is something changed in fixcc.py.  I'll check
there.

hmm... ok, that file is difficult to read.  But gives me an
idea... what if I just run fixcc.py on the output of astyle
--style=gnu ?

... run the command... git-diff... look at the patch... ok, it's
bigger now.  that sucks.  What happens if we skip astyle and just
run fixcc.py ?

... ok, it *still* gives a 3-meg patch.  This is pretty good
evidence that, despite some people's claims to the contrary,
fixcc.py is broken.  Let's give up on that.


9)  ok, back to the original "let's use 2 spaces instead of 4".
  astyle --indent=spaces=2 *.cc
address@hidden:~/src/lilypond/lily$ ls -lh foo.patch 
-rw-r--r-- 1 gperciva gperciva 2.2M 2009-11-13 17:30 foo.patch

well, this is looking better.  BTW, it's interesting that the
"non-GNU" style works better than the GNU style.

So, what's in the diff... hmm, stuff like this:
--- a/lily/accidental-engraver.cc
+++ b/lily/accidental-engraver.cc
@@ -109,8 +109,8 @@
Accidental_engraver::update_local_key_signature (SCM new_sig)
 {
   last_keysig_ = new_sig;
   set_context_property_on_children (context (),
-                                   ly_symbol2scm
                                    ("localKeySignature"),
-                                   new_sig);
+                                    ly_symbol2scm
("localKeySignature"),
+                                    new_sig);

   Context *trans = context ()->get_parent_context ();



that looks ok to me.  (although the email might break it up).
Basically, instead of lining up
  (context
  ly_symbol2cm
it lines up
  (contest
   ly_symbol2cm

I think the original was actually a typo.  Ok.  I am comfortable
proposing (and defending) this change.

what else is there?
@@ -121,10 +121,10 @@
Accidental_engraver::update_local_key_signature (SCM new_sig)

   SCM val;
   while (trans && trans->where_defined (ly_symbol2scm
("localKeySignature"), &val) == trans)
-    {
-      trans->set_property ("localKeySignature", ly_deep_copy
       (last_keysig_));
-      trans = trans->get_parent_context ();
-    }
+  {
+    trans->set_property ("localKeySignature", ly_deep_copy
(last_keysig_));
+    trans = trans->get_parent_context ();
+  }
 }

 struct Accidental_result


aha, the dreaded "where should { brace go" ?  well, astyle has a
whole bunch of options to change this.  For now, I'll just make a
note that we need to discuss this.  I'll keep on going, but once
I've finished my investigations, I'll make an email thread
specifically for this example, asking what we want, and telling
people that astyle can do whatever they decide they want.

Then I can sit back with popcorn and watch the debate.  HOWEVER,
I'm not going to start that discussion unless people have a reason
to trust that I'm going to follow through.


What's another thing... aha:
@@ -152,14 +152,14 @@ struct Accidental_result
   int score () const
   {
     return need_acc ? 1 : 0
-      + need_restore ? 1 : 0;
+           + need_restore ? 1 : 0;
   }
 };

this seems like a pretty fiddly change, but whatever.  I would be
happy to defend it.



let's skip on a bit.  Hmm, there's *huge* bunches of text that's
moved one space to the right because of lining up under the first
argument, rather than lining up under the (.  So although a 2-meg
patch *sounds* scary, it actually doesn't change all that much.



since this is just an example, I'm going to skip to about halfway
through this patch.

... let's see, more "where should the brace start"... more "where
should an argument on the next line be placed"... hmm, here's a
new one:
   Font_metric *musfont
-    = select_font (me->layout (), alist_chain);
+  = select_font (me->layout (), alist_chain);

I don't particularly like the look of that.  I wonder if there's
another option we could give to astyle to make it line up the = as
per the old code?

hmm, I can't see one.  Now, I'm not comfortable defending this
change.  Having
  Font_metric *musfont
  = select_font (me->...
just doesn't make sense.  So either I need to double-check the
available options, file a bug report / feature request with
astyle, or start looking for another tool.



Oh, by the way... does astyle run on windows?  It would be pretty
silly if we chose a tool that only runs on unix machines, when
everybody's complaining about the difficulty of developing on
windows.

maybe astyle isn't the best choice after all.  I mean, I haven't
personally looked at any alternatives; there might be another tool
that *does* run on windows.  Or maybe astyle has a windows binary
available.



Whatever.  You're giving up.  All this time I've spent trying to
help you on this was wasted.  I'm sure that more experienced
developers list are either laughing at me, or shaking
their heads sadly... "oh that Graham, he's trying to help the
beginners, but he still doesn't understand that it's just not
worth it"

- Graham