Merge lp:~michihenning/unity-api/astyle into lp:unity-api

[Michi Henning]

Added astyle-config.

# Options for formatting code with astyle.
#
# This helps to make code match the style guide.
#
# Use like this:
#
# astyle --options=astyle-config mfile.h myfile.cpp
#
# Occasionally, astyle does something silly (particularly with lambdas), so it's
# still necessary to scan the changes for things that are wrong.
# But, for most files, it does a good job.
#
# Please consider using this before checking code in for review. Code reviews shouldn't
# have to deal with layout issues, they are just a distraction. It's better to be able
# to focus on semantics in a code review, with style issues out of the way.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:66
http://jenkins.qa.ubuntu.com/job/unity-api-ci/89/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-saucy-armhf-ci/46
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-saucy-armhf-ci/46/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-saucy-i386-ci/47

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/89/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

Maybe a "make style" / "make astyle" target for easy invocation?

review: Needs Information

[Michi Henning (michihenning)]

> Maybe a "make style" / "make astyle" target for easy invocation?

I considered doing this but figured that it probably wouldn't do any good. That's because astyle isn't perfect and occasionally messes with things that are reasonable. So, if I run astyle recursively over the tree, it'll do lots of good things once, which is nice. But if I run it over a tree that's in perfectly good shape, it repeatedly will make the same mess of some things.

If astyle were perfect, I wouldn't hesitate to run it automatically as part of the tool chain. But, unfortunately, it isn't :-(

[Michał Sawicz (saviq)]

W dniu 20.06.2013 03:17, Michi Henning pisze:
> I considered doing this but figured that it probably wouldn't do any good. That's because astyle isn't perfect and occasionally messes with things that are reasonable. So, if I run astyle recursively over the tree, it'll do lots of good things once, which is nice. But if I run it over a tree that's in perfectly good shape, it repeatedly will make the same mess of some things.

I'm not saying that it should be run on "all" target, but instead of
having to remember the astyle --... command, a quick make would maybe be
used more easily / more often?

We could try and find out the changed files (but then without checking
against :parent branch it would only style whatever wasn't committed yet).

--
Michał Sawicz <email address hidden>
Canonical Services Ltd.

[Michi Henning (michihenning)]

> I'm not saying that it should be run on "all" target, but instead of
> having to remember the astyle --... command, a quick make would maybe be
> used more easily / more often?

Hmmm... What should "make astyle" in a given directory do?

astyle requires a list of source and header files. If I add --recursive, it also descends into sub-directories.

So, if I'm in src/unity/util, how would I tell astyle to not mess with files that have, say, a lambda in them that might get re-written poorly?

> We could try and find out the changed files (but then without checking
> against :parent branch it would only style whatever wasn't committed yet).

astyle reports which files it touched. But I'm not sure that helps. Suppose there is source file that contains a lambda that gets messed up by astyle. I make a change to the source file, but I also break the style guide somewhere. Now, when I run astyle, it'll fix my broken style, but mess up the lambda.

So, it seems that "make astyle" would repeatedly do damage for some files in a directory, even without --recursive? Or am I missing something else here?

[Michi Henning (michihenning)]

Any more comments on this? I'm not sure how to progress this.

[Jussi Pakkanen (jpakkane)]

Fine by me. We can add the target invocation in a different MR. Michał?

review: Approve

[Michał Sawicz (saviq)]

W dniu 20.06.2013 03:44, Michi Henning pisze:
>> I'm not saying that it should be run on "all" target, but instead of
>> having to remember the astyle --... command, a quick make would maybe be
>> used more easily / more often?
>
> Hmmm... What should "make astyle" in a given directory do?
>
> astyle requires a list of source and header files. If I add --recursive, it also descends into sub-directories.
>
> So, if I'm in src/unity/util, how would I tell astyle to not mess with files that have, say, a lambda in them that might get re-written poorly?
>
>> We could try and find out the changed files (but then without checking
>> against :parent branch it would only style whatever wasn't committed yet).
>
> astyle reports which files it touched. But I'm not sure that helps. Suppose there is source file that contains a lambda that gets messed up by astyle. I make a change to the source file, but I also break the style guide somewhere. Now, when I run astyle, it'll fix my broken style, but mess up the lambda.
>
> So, it seems that "make astyle" would repeatedly do damage for some files in a directory, even without --recursive? Or am I missing something else here?

I thought the other way around - find out which files were changed, and
only run astyle on them. It's obviously problematic when you've already
committed before (we might try and compare against :parent, but that
could be unreliable...).

> Any more comments on this? I'm not sure how to progress this.

> Fine by me. We can add the target invocation in a different MR. Michał?

Yeah, let's.

--
Michał (Saviq) Sawicz <email address hidden>
Canonical Services Ltd.

[Michał Sawicz (saviq)]

review: Approve

[Michi Henning (michihenning)]

> I thought the other way around - find out which files were changed, and
> only run astyle on them. It's obviously problematic when you've already
> committed before (we might try and compare against :parent, but that
> could be unreliable...).

Not pretty. Probably more complex than what it is worth. And the problem still remains: if I change a file that contains something that astyle will mess up, I'm screwed even if my style is perfect.

For now, I think it's best to run this as a manual step. People can feed their code through style before proposing the merge, and a quick check with bar diff will tell them whether astyle changed anything that it should have left alone.

=== modified file 'README'
--- README	2013-06-12 22:07:05 +0000
+++ README	2013-06-20 00:21:34 +0000
@@ -211,6 +211,12 @@
 Things that are dubious in some way with no better solution available
 are marked with a HACK comment.
 
+Style
+-----
+
+Consider running astyle over the code before checking it in.
+See astyle-config for more details.
+
 
 Test suite
 ----------

=== added file 'astyle-config'
--- astyle-config	1970-01-01 00:00:00 +0000
+++ astyle-config	2013-06-20 00:21:34 +0000
@@ -0,0 +1,44 @@
+# Options for formatting code with astyle.
+#
+# This helps to make code match the style guide.
+# 
+# Use like this:
+# 
+#     astyle --options=astyle-config mfile.h myfile.cpp
+# 
+# Occasionally, astyle does something silly (particularly with lambdas), so it's
+# still necessary to scan the changes for things that are wrong.
+# But, for most files, it does a good job.
+# 
+# Please consider using this before checking code in for review. Code reviews shouldn't
+# have to deal with layout issues, they are just a distraction. It's better to be able
+# to focus on semantics in a code review, with style issues out of the way.
+
+--formatted
+--style=allman
+--min-conditional-indent=2
+--indent-switches
+--max-instatement-indent=80
+--pad-header
+--align-pointer=type
+--align-reference=type
+--add-brackets
+--convert-tabs
+--close-templates
+--max-code-length=132
+
+# --pad-oper
+#
+# Commented out for now. It changes
+#
+#     for (int i=0; i<10; ++i)
+# to
+#     for (int i = 0; i < 10; ++i)
+#
+# Unfortunately, it also messes with rvalue references:
+#
+#     ResourcePtr& operator=(ResourcePtr&& r);
+#
+# becomes:
+#
+#     ResourcePtr& operator=(ResourcePtr && r);