Hi Akos,

Akos Vandra wrote:
> Bingo, I am working on Ubuntu 10.10, x64, gcc 4.4.5 (shipped with ubuntu).

10.10 is soon one year old, which can be a long time in open source.

Also keep in mind that pretty much every major distribution is
applying significant amounts of patches in their toolchain packages
and possibly also system libraries.

They intend this to make things better of course, but it can backfire
and make the toolchain unusable sometimes. Keep this in mind if you
encounter a weird build issue of any package.

>From many years of use of gcc and binutils I would recommend to use
a vanilla toolchain, or at the very least very aggressively pulling
toolchain updates from your distribution.


> I have been getting some warnings (treated as errors) due to some
> retvals didn't have initial value set, but I set them to ERROR_OK,
> and now it passes those points.

Please send patch(es) for these to the mailing list. Thanks!


> It seems like uint32_t is not defined, even though it should be, as
> stdint.h is included.

stdint.h is included only #ifdef HAVE_STDINT_H. Did you verify with
gcc -E or at least in config.h that HAVE_STDINT_H is defined?


> Funny thing, if I set the first occurance of uint32_t to unsigned
> int, the other ones build fine, which is kinda wierd.

It is not funny at all. I find it typical of a toolchain issue. It is
bullshit that wastes precious developer time. :\


> Also, should we spamming the developer list,

Certainly yes. The purpose of the developer list is to carry
developer discussion.


> or rather continue this discussion personally?

Absolutely not.


> I'm not familiar with opensource development *at all*,

Welcome to open source! It can be a wonderful environment of
cooperation and synergy when things align well. It can also be a
challenging environment of conflicts and complaints when they don't.

Overall I personally find it to be more than worth the trouble, and
I think it leads to better software.


> so I'm not familiar with list policies either.

Again, this is a developer list whose sole purpose is for developer
communication. Direct communication should be an exception.
(Sometimes it's the right thing of course, just usually it is not.)

Please study git and work on becoming fluent with it if you are not
already. It is a great tool that supports development in very many
ways. Beyond simply keeping track of what happened in the code git
when used well allows incredibly efficient review and movement of
code between developers in a project. I can recommend the
http://progit.org/ book, but start out by just playing with it a
little. Make some repositories with some commits and get the basic
workflow going. Learning the rest is usually easy. I also recommend
the #git IRC channel on freenode, for getting live help.


One use for developer mailing lists is to distribute proposed
patches, so that they can be reviewed and then included into the
public source tree.

To make the review step quick and thus make it somewhat likely to
actually happen, it is *critical* that the patch is what I call
"clean". Maybe all this is obvious to you already, in that case
please skip to bottom. The individual points of a clean patch are:


* Write a top quality commit message, technically and logically

In git repositories it is useful to follow a certain commit message
format: (Line numbers added to the left)

1. Description of change, prefer <=60 chars, absmax 74 or even 72 chars
2. 
3. Third line and onward hold longer description of change
4. each line absmax 74 chars

The first line is used in list views, where one commit is listed per
line. This makes it important to have a short and sweet description
of the change in the first line. The second line is always left
blank. The third line and any number of following lines can and
should include background on this part of the code and discuss why
this change was made the way it was done instead of some other more
obvious way. Ie. document the research that went into this particular
change of the code. Including links to web pages with relevant
information is helpful, as is links to mailing list messages e.g. on
http://marc.info/ which has nice short message links. (Only the m=
URL parameter is required for links to a message.)

The line length restrictions are in part due to use of 80 char wide
terminals, but more importantly they are due to how patches are sent
and refered to in email. I.e. even if in an open source project no
active developer is using 80 char wide terminals there may be
participants on the mailing list who benefit from the shorter lines,
in which case it makes sense to keep them short. Also, web based
repository viewers tend to not want or be able to present very long
first line descriptions.

As for the logical quality, it is important to write the first line
description of the change so that it makes sense for someone who
knows nothing at all about the code, since this is used in list
views, and since the background for this code and for why this change
was done the way it was done comes only in the later lines, which may
not be available from where that list view is. On a web page of
course it is, but a list of commits can easily appear in an email,
which is read by someone offline on an airplane. A description
refering to code they don't have and don't know is useless. Keep it
high level, clear and simple. Writing this one line is not
neccessarily easy.

Writing the third line and onwards is usually easier. Before creating
this commit you started at some point, faced with a task, a problem
or even a bug ticket, and you then learned some things in order to
make the committed code change. Basically you want to make sure that
a developer doing review also learns the same things, so that they
are able to adequatly review your conclusions, without having to
duplicate the time you spent on learning how and why to make the
change. A bonus for reviewers is that they learn a lot from code
written by others.


* Change only one thing per commit

>From the above discussion on commit message it is clear that in order
to write a useful commit message it is critical that one commit only
includes one logical change. Otherwise it's impossible to write a one
line summary.

So what's a "thing" - well any logical change, small or big. It can
be about renaming variables in order to prepare for a later commit
which does code refactoring. It can also be one bug fix which touches
a lot of different places in the code. The developer creating the
commit decides on what is the thing, and indeed one part of review is
for other developers to give feedback on how their view of this thing
align. Sometimes this will lead to reworking the patch to solve the
same problem with other logical steps in the code.

Besides being common sense, the one thing per commit rule is also
technically important, e.g. in order for the git bisect feature to be
meaningful, and also for being able to later revert a logical change
which has turned out to be a bad idea after all.


* Adhere strictly to code style where you work

A commit can change lots of different places in the code and for
historical reasons it's totally possible that they use different code
style. Per the previous point about one thing per commit any code
style changes should be a separate commit, even if it is tempting
also for me to clean up code when I am anyway working in that area.
This is fine, but the critical point is to not commit them together.
git add -p will allow to post process changes in a file, before they
get committed, so that you can commit style change/cleanup
separately. In general, reduce code style changes to an absolute
minimum. Not only are style changes very demanding to review (since
they usually change large amounts of code) but style changes also
clutter repository history, which (and this is important) makes it a
lot more work to go back and look at the history of a piece of code,
e.g. to compare what the code did a year ago when it worked, with
what it does now when it has regressed. A big style change of the
code adds enormous overhead to fixing this problem. Get style right
the first time. Hopefully review will catch problems, but be careful
to try to avoid code style issues.


* Do review yourself before creating a commit

With git it is amazingly easy to perform review of the changes one
has made, using git diff. Always use this to look for all of the
above points, before you create the commit. When working in the
source files it is easy to get tunnel vision and make some small
change that gets forgotten later, when focusing on a particular
problem. When looking at git diff output that small change is so
obvious, and it can be taken care of without requiring a round trip
to any other developer.


* Do review yourself before sending commits to anyone else

Especially for long gnarly sessions leading to a commit it's possible
to still miss something. Reviewing the changes before posting is so
easy because they are available as commits in the local repository.
git log -p shows each commit with commit message and all changes to
the code, and assuming your terminal supports it the output will even
be colour coded. Do this before you send a patch to the mailing list
(e.g. using git send-email) for review by others, again saving time
for others and reducing round trips.


This might seem like an aweful lot to keep in mind just for working
on some code, but I hope that I was able to give good reasons for
each point and if they are summarized, actually it doesn't look
like such a mouthful anymore:

* Write a top quality commit message, technically and logically
* Change only one thing per commit
* Adhere strictly to code style where you work
* Do review yourself before creating a commit
* Do review yourself before sending commits to anyone else


I hope this makes sense. Please feel free to ask any questions.


//Peter
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development

Reply via email to