[xen 4.6 retrospective] [bad] Code style checking takes up too much time

[xen 4.6 retrospective] [bad] Code style checking takes up too much time

[Lars Kurth]

Another one from the developer meeting

= Issue / Observation =
Code style checking takes up too much time. This affects both reviewers who spend a significant amount commenting on style issues and also makes it harder to for contributors to focus on non-style issues. What I mean by the latter is that the volume of comments that need to be tracked by contributors, makes it harder for them.

= Possible Solution / Improvement =
Automate code-style checks through a tool, that could be run as a pre-patch review tool. Once we have that, update contribution docs.

As far as I recall, Andy Cooper believes that dealing with the 2 different coding styles should not be a huge issue. A good tool could differentiate by directory / file.

What may be a bigger issue, is that older code may not fully adhere to coding standards. The open question is then
* Whether such a tool should only run on the diff/patch
* If not, how we deal with check failures in areas of a file that have not been touched by the contributor

Re: [xen 4.6 retrospective] [bad] Code style checking takes up too much time

[Jan Beulich]

>>> On 28.08.15 at 17:39, <lars.kurth.xen@gmail.com> wrote:
> What may be a bigger issue, is that older code may not fully adhere to 
> coding standards. The open question is then
> * Whether such a tool should only run on the diff/patch

Isn't that the intended / expected behavior of such a tool anyway?
If so, the real problem here is how the tool should determine the
style of a file being modified when that file's style isn't reasonably
clean.

> * If not, how we deal with check failures in areas of a file that have not 
> been touched by the contributor

Manual inspection, using common sense to decide whether to ignore
the tools complaint?

Jan

Re: [xen 4.6 retrospective] [bad] Code style checking takes up too much time

[Andrew Cooper]

On 31/08/15 08:27, Jan Beulich wrote:
>>>> On 28.08.15 at 17:39, <lars.kurth.xen@gmail.com> wrote:
>> What may be a bigger issue, is that older code may not fully adhere to 
>> coding standards. The open question is then
>> * Whether such a tool should only run on the diff/patch
> Isn't that the intended / expected behavior of such a tool anyway?

I would have thought so, and moreso only on the newly added lines.

> If so, the real problem here is how the tool should determine the
> style of a file being modified when that file's style isn't reasonably
> clean.

As we tag most files with an emacs magic block, it would be fine to have
an extra tag used by an automatic tool.

It might also be an idea for someone who uses vim to provide a vim magic
block which we also use.  That might help to reduce the effort required
by contributors to get it right first time.

>
>> * If not, how we deal with check failures in areas of a file that have not 
>> been touched by the contributor
> Manual inspection, using common sense to decide whether to ignore
> the tools complaint?

Ideally we would engineer such a tool to have as few false results as
possible, but there are some cases where it might be preferable (e.g.
changing the indentation of a large block) to keep the old style to aid
the clarity of the current patch.  This will need to be decided on a
case-by-case basis.

~Andrew

Re: [xen 4.6 retrospective] [bad] Code style checking takes up too much time

[Ian Campbell]

On Mon, 2015-08-31 at 14:24 +0100, Andrew Cooper wrote:
> On 31/08/15 08:27, Jan Beulich wrote:
> > > > > On 28.08.15 at 17:39, <lars.kurth.xen@gmail.com> wrote:
> > > What may be a bigger issue, is that older code may not fully adhere 
> > > to 
> > > coding standards. The open question is then
> > > * Whether such a tool should only run on the diff/patch
> > Isn't that the intended / expected behavior of such a tool anyway?
> 
> I would have thought so, and moreso only on the newly added lines.

That's what Linux's checkpatch.pl does, I think.