[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Code formatter

From: Bertalan Fodor (LilyPondTool)
Subject: Re: Code formatter
Date: Fri, 13 Nov 2009 19:07:38 +0100
User-agent: Thunderbird (Windows/20090812)

Forget the tool. Astyle is very configurable, but we still could need our own formatter (though I wouldn't like that).
Developing C++ code on Windows is practically impossible, but can be well done with the VirtualBox pc for which I host the ISO (see the CG :))
So it doesn't matter whether the tool is supported on Windows or not.

But first:
- what are the rules?
- and then how we force that
You can even force that with manually, that is not approving code that doesn't adhere, but all that can be automatized could be. So there might be some static code analysis tools to be used as well.

Graham Percival wrote:
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/ 
+++ b/lily/ 
@@ -29,61 +29,61 @@ 
 class Accidental_entry 
-  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/
+++ b/lily/
@@ -27,18 +27,18 @@
 #include "translator.icc"

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

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

hmm... ok, that file is difficult to read.  But gives me an
idea... what if I just run 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 ?

... ok, it *still* gives a 3-meg patch.  This is pretty good
evidence that, despite some people's claims to the contrary, 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/
+++ b/lily/
@@ -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
-                                   new_sig);
+                                    ly_symbol2scm
+                                    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
it lines up

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
-      trans = trans->get_parent_context ();
-    }
+  {
+    trans->set_property ("localKeySignature", ly_deep_copy
+    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

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

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

lilypond-devel mailing list


reply via email to

[Prev in Thread] Current Thread [Next in Thread]