* CodingStyle indentation and alignment
@ 2016-09-26 17:28 Laurence Rochfort
2016-09-26 19:32 ` Andrey Utkin
2016-09-27 10:26 ` Bernd Petrovitsch
0 siblings, 2 replies; 6+ messages in thread
From: Laurence Rochfort @ 2016-09-26 17:28 UTC (permalink / raw)
To: kernelnewbies
Hi all,
I've read Documentation/CodingStyle and it states to use 8 character tabs.
Reading several USB driver files including drivers/usb/usb-skeleton.c, I see that multi-line lists of argument and operands are often aligned on top of each other using a mixture of tabs and spaces. checkpatch doesn't complain about the mixture.
For instance from usb-skeleton.c:
static int skel_probe(struct usb_interface *interface,
const struct usb_device_id *id)
uses two tabs and 6 spaces, not just tabs like:
static int skel_probe(struct usb_interface *interface,
const struct usb_device_id *id)
or
static int skel_probe(struct usb_interface *interface,
const struct usb_device_id *id)
Is a mixture of tabs and spaces acceptable if it enhances readability? If not, which of the tabs-only forms is correct?
Similarly, what about assignment alignment in structs?
Cheers,
Laurence.
^ permalink raw reply [flat|nested] 6+ messages in thread
* CodingStyle indentation and alignment
2016-09-26 17:28 CodingStyle indentation and alignment Laurence Rochfort
@ 2016-09-26 19:32 ` Andrey Utkin
2016-09-27 14:27 ` Laurence Rochfort
2016-09-27 10:26 ` Bernd Petrovitsch
1 sibling, 1 reply; 6+ messages in thread
From: Andrey Utkin @ 2016-09-26 19:32 UTC (permalink / raw)
To: kernelnewbies
On Mon, Sep 26, 2016 at 06:28:58PM +0100, Laurence Rochfort wrote:
> Hi all,
>
> I've read Documentation/CodingStyle and it states to use 8 character tabs.
>
> Reading several USB driver files including drivers/usb/usb-skeleton.c, I see that multi-line lists of argument and operands are often aligned on top of each other using a mixture of tabs and spaces. checkpatch doesn't complain about the mixture.
>
> For instance from usb-skeleton.c:
>
> static int skel_probe(struct usb_interface *interface,
> const struct usb_device_id *id)
>
> uses two tabs and 6 spaces, not just tabs like:
>
> static int skel_probe(struct usb_interface *interface,
> const struct usb_device_id *id)
>
> or
>
> static int skel_probe(struct usb_interface *interface,
> const struct usb_device_id *id)
>
>
> Is a mixture of tabs and spaces acceptable if it enhances readability? If not, which of the tabs-only forms is correct?
>
> Similarly, what about assignment alignment in structs?
By my observations, conventional alignment in kernel contributions
nowadays is N*tab followed by M*space, where M < 8. For details, consult
"checkpatch --strict" output. It's not a paramount to make all existing
codebase to pass that check cleanly, but if you are polishing your new
patch, you can use that as guidance.
If you use Vim, see https://github.com/vivien/vim-linux-coding-style -
it will indent your code more or less in accordance with coding style,
also making checkpatch happy.
checkpatch in --strict mode actually prefers trailing lines to be
aligned on opening brace. Tabs and spaces are allowed for such alignment
(and you _need_ spaces to be able to align like that).
So what above vim plugin makes, and what is widely used in kernel
codebase, is something like this ("|tab--->" or a smaller part
represents tab character, "." represents space):
static int blah_blah_blah(struct usb_interface *interface,
|tab--->|tab--->|tab--->..const struct usb_device_id *id)
{
|tab--->if (1) {
|tab--->|tab--->int var = blah_blah(interface->foo_bar->baz_fuzz,
|tab--->|tab--->|tab--->|tab--->....11111111111111 + 111111111111111);
Also
|tab--->|tab--->|tab--->|tab--->|tab--->|tab--->|tab---> (shown for reference)
#define DRIVERNAME_REGISTER1_NAMEXXX--->|tab--->0xdead
#define DRIVERNAME_REGISTER2_NAME_WHICH_IS_LONG>0xdeaf
Which looks this way with actual tab characters:
#define DRIVERNAME_REGISTER1_NAMEXXX 0xdead
#define DRIVERNAME_REGISTER2_NAME_WHICH_IS_LONG 0xdeaf
This is supposed to be displayed ALWAYS with 8-char tab (this is carved
in slate of CodingStyle), thus arguments about breaking viewing with
different tab sizes are to be ignored.
To my taste this is not best possible approach, and I would like all
readers of this thread to check this article:
http://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces
But, unfortunately, checkpatch is explicitly not happy with that
approach, giving notices of ERROR and WARNING levels for first example,
however accepting #define case to be aligned with spaces IIRC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* CodingStyle indentation and alignment
2016-09-26 17:28 CodingStyle indentation and alignment Laurence Rochfort
2016-09-26 19:32 ` Andrey Utkin
@ 2016-09-27 10:26 ` Bernd Petrovitsch
1 sibling, 0 replies; 6+ messages in thread
From: Bernd Petrovitsch @ 2016-09-27 10:26 UTC (permalink / raw)
To: kernelnewbies
Hia ll!
On Mon, 2016-09-26 at 18:28 +0100, Laurence Rochfort wrote:
>?[...]
> I've read Documentation/CodingStyle and it states to use 8 character
> tabs.
That means, that a <tab> uses up to eight columns and not that one
should replacre some 8 spaces somewhere with a <tab> or that spaces are
not allowed.
> Reading several USB driver files including drivers/usb/usb-
> skeleton.c, I see that multi-line lists of argument and operands are
> often aligned on top of each other using a mixture of tabs and
> spaces. checkpatch doesn't complain about the mixture.
Why should that be a problem.
> For instance from usb-skeleton.c:
>
> static int skel_probe(struct usb_interface *interface,??????????????????????????????
> ??????????????????????const struct usb_device_id *id)
>
> uses two tabs and 6 spaces, not just tabs like:
>
> static int skel_probe(struct usb_interface *interface,
> ??????? ???const struct usb_device_id *id)
>
> or
>
> static int skel_probe(struct usb_interface *interface,
> ??????? ??? ?????const struct usb_device_id *id)
Which of the three versions is most readable/consistent?
IMHO the first (and it is the only consistent).
Consider more than 2 paramaters which I align as in
---- ?snip ?----
static int foobarfunc(struct usb_interface *interface,
const struct usb_device_id *id,
int whatever,
const char *and_a_string)?
---- ?snip ?----
Would it be "better" as
---- ?snip ?----
static int foobarfunc(
struct usb_interface *interface,
const struct usb_device_id *id,
int
whatever,
const char *and_a_string
)
----
?snip ?----
(which is also somewhat used somewhere)?
That wastes two
lines IMHO.
Same for
---- ?snip ?----
static int
foobarfunc(
struct usb_interface *interface,
const struct usb_device_id *id,
int
whatever,
const char *and_a_string
)?
----
?snip ?----
which has the motivation "I can grep the function body with
^functionname" (but in the light of IDEs and `ctags` isn't that
important anymore).
And remember that we are here for 99% in the "what one is used to" department.
> Is a mixture of tabs and spaces acceptable if it enhances
> readability?
Only readability is really important - not some document (describing
the rough direction to go an gives some simple direct hints for the
undisputed details) or output of some simple script/tool which tries to
detect violations[0].
Both of them are just guidelines and do not justify a single patch
stupidly and blindly just "correcting" some checkpatch complaint.
Actually "checkpatch" needs some "do not warn in the next n lines"
mechanism (or similar, e.g. `indent` and probably all source code
formatters hava that too) so that known/intended violations for good
reason are silenced.
Bernd
[0]: And "checkpatch" is a good tool but it is just a tool and one?
? ? ?needs to see the source to decide if it's better - not the simple?
? ? ?warning of eome reg-exp.
--
Bernd Petrovitsch Email : bernd at petrovitsch.priv.at
LUGA : http://www.luga.at
^ permalink raw reply [flat|nested] 6+ messages in thread
* CodingStyle indentation and alignment
2016-09-26 19:32 ` Andrey Utkin
@ 2016-09-27 14:27 ` Laurence Rochfort
2016-09-27 14:47 ` Andrey Utkin
0 siblings, 1 reply; 6+ messages in thread
From: Laurence Rochfort @ 2016-09-27 14:27 UTC (permalink / raw)
To: kernelnewbies
Thanks very much for the comprehensive answer.
Using spaces and tabs for alignment does make the code more readable
to my mind.
The reason behind my question originally was that the CodingStyle doc
states:
"Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation, and the above example is deliberately broken."
however most code I have seen is not compliant with that.
Should the docs be updated?
Cheers,
Laurence.
On Mon, Sep 26, 2016 at 10:32:06PM +0300, Andrey Utkin wrote:
> On Mon, Sep 26, 2016 at 06:28:58PM +0100, Laurence Rochfort wrote:
> > Hi all,
> >
> > I've read Documentation/CodingStyle and it states to use 8 character
tabs.
> >
> > Reading several USB driver files including drivers/usb/usb-skeleton.c,
I see that multi-line lists of argument and operands are often aligned on
top of each other using a mixture of tabs and spaces. checkpatch doesn't
complain about the mixture.
> >
> > For instance from usb-skeleton.c:
> >
> > static int skel_probe(struct usb_interface *interface,
> > const struct usb_device_id *id)
> >
> > uses two tabs and 6 spaces, not just tabs like:
> >
> > static int skel_probe(struct usb_interface *interface,
> > const struct usb_device_id *id)
> >
> > or
> >
> > static int skel_probe(struct usb_interface *interface,
> > const struct usb_device_id *id)
> >
> >
> > Is a mixture of tabs and spaces acceptable if it enhances readability?
If not, which of the tabs-only forms is correct?
> >
> > Similarly, what about assignment alignment in structs?
>
> By my observations, conventional alignment in kernel contributions
> nowadays is N*tab followed by M*space, where M < 8. For details, consult
> "checkpatch --strict" output. It's not a paramount to make all existing
> codebase to pass that check cleanly, but if you are polishing your new
> patch, you can use that as guidance.
>
> If you use Vim, see https://github.com/vivien/vim-linux-coding-style -
> it will indent your code more or less in accordance with coding style,
> also making checkpatch happy.
>
> checkpatch in --strict mode actually prefers trailing lines to be
> aligned on opening brace. Tabs and spaces are allowed for such alignment
> (and you _need_ spaces to be able to align like that).
>
> So what above vim plugin makes, and what is widely used in kernel
> codebase, is something like this ("|tab--->" or a smaller part
> represents tab character, "." represents space):
>
> static int blah_blah_blah(struct usb_interface *interface,
> |tab--->|tab--->|tab--->..const struct usb_device_id *id)
> {
> |tab--->if (1) {
> |tab--->|tab--->int var = blah_blah(interface->foo_bar->baz_fuzz,
> |tab--->|tab--->|tab--->|tab--->....11111111111111 + 111111111111111);
>
> Also
>
> |tab--->|tab--->|tab--->|tab--->|tab--->|tab--->|tab---> (shown for
reference)
> #define DRIVERNAME_REGISTER1_NAMEXXX--->|tab--->0xdead
> #define DRIVERNAME_REGISTER2_NAME_WHICH_IS_LONG>0xdeaf
>
> Which looks this way with actual tab characters:
>
> #define DRIVERNAME_REGISTER1_NAMEXXX 0xdead
> #define DRIVERNAME_REGISTER2_NAME_WHICH_IS_LONG 0xdeaf
>
> This is supposed to be displayed ALWAYS with 8-char tab (this is carved
> in slate of CodingStyle), thus arguments about breaking viewing with
> different tab sizes are to be ignored.
>
> To my taste this is not best possible approach, and I would like all
> readers of this thread to check this article:
> http://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces
> But, unfortunately, checkpatch is explicitly not happy with that
> approach, giving notices of ERROR and WARNING levels for first example,
> however accepting #define case to be aligned with spaces IIRC.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20160927/3982944c/attachment-0001.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* CodingStyle indentation and alignment
2016-09-27 14:27 ` Laurence Rochfort
@ 2016-09-27 14:47 ` Andrey Utkin
2016-09-27 14:47 ` Laurence Rochfort
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Utkin @ 2016-09-27 14:47 UTC (permalink / raw)
To: kernelnewbies
On Tue, Sep 27, 2016 at 02:27:27PM +0000, Laurence Rochfort wrote:
> Thanks very much for the comprehensive answer.
>
> Using spaces and tabs for alignment does make the code more readable
> to my mind.
>
> The reason behind my question originally was that the CodingStyle doc
> states:
>
> "Outside of comments, documentation and except in Kconfig, spaces are never
> used for indentation, and the above example is deliberately broken."
>
> however most code I have seen is not compliant with that.
>
> Should the docs be updated?
There's difference between terms "indentation" and "alignment".
Please see paragraph "Indentation and alignment" of previously
referenced article:
http://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces#indentation_and_alignment
Accounting this difference, the doc is correct in strict sense.
^ permalink raw reply [flat|nested] 6+ messages in thread
* CodingStyle indentation and alignment
2016-09-27 14:47 ` Andrey Utkin
@ 2016-09-27 14:47 ` Laurence Rochfort
0 siblings, 0 replies; 6+ messages in thread
From: Laurence Rochfort @ 2016-09-27 14:47 UTC (permalink / raw)
To: kernelnewbies
Good point.
On Tue, Sep 27, 2016 at 3:47 PM Andrey Utkin <andrey_utkin@fastmail.com>
wrote:
> On Tue, Sep 27, 2016 at 02:27:27PM +0000, Laurence Rochfort wrote:
> > Thanks very much for the comprehensive answer.
> >
> > Using spaces and tabs for alignment does make the code more readable
> > to my mind.
> >
> > The reason behind my question originally was that the CodingStyle doc
> > states:
> >
> > "Outside of comments, documentation and except in Kconfig, spaces are
> never
> > used for indentation, and the above example is deliberately broken."
> >
> > however most code I have seen is not compliant with that.
> >
> > Should the docs be updated?
>
> There's difference between terms "indentation" and "alignment".
> Please see paragraph "Indentation and alignment" of previously
> referenced article:
>
> http://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces#indentation_and_alignment
>
> Accounting this difference, the doc is correct in strict sense.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20160927/85cc00c8/attachment.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-27 14:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-26 17:28 CodingStyle indentation and alignment Laurence Rochfort
2016-09-26 19:32 ` Andrey Utkin
2016-09-27 14:27 ` Laurence Rochfort
2016-09-27 14:47 ` Andrey Utkin
2016-09-27 14:47 ` Laurence Rochfort
2016-09-27 10:26 ` Bernd Petrovitsch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.