* Bluez coding style again
@ 2009-01-17 17:43 Siarhei Siamashka
2009-01-18 15:08 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Siarhei Siamashka @ 2009-01-17 17:43 UTC (permalink / raw)
To: BlueZ development
Hi,
> commit 1fb5ffff0b704038a90e03860ecce63be2deb988
> Author: Johan Hedberg <johan.hedberg@nokia.com>
> Date: Fri Jan 16 20:29:43 2009 +0200
>
> Fix indentation to use only tabs
>
> --- a/sbc/sbc_tables.h
> +++ b/sbc/sbc_tables.h
> @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> #define F(x) F_PROTO4(x)
> static const FIXED_T _sbc_proto_fixed4[40] = {
> - F(0.00000000E+00), F(5.36548976E-04),
> + F(0.00000000E+00), F(5.36548976E-04),
> -F(1.49188357E-03), F(2.73370904E-03),
> - F(3.83720193E-03), F(3.89205149E-03),
> - F(1.86581691E-03), F(3.06012286E-03),
> + F(3.83720193E-03), F(3.89205149E-03),
> + F(1.86581691E-03), F(3.06012286E-03),
With this latest commit, the vertical alignment of elements in tables gets
messed up and it becomes harder to see any kind of symmetry in the tables.
If the spaces are strictly forbidden, I would probably prefer replacing them
with '+' character.
Anyway, with this requirement also in effect, it makes harder to use the
standard 'checkpatch.pl' from linux distribution as you have to watch for
a lot more stuff which can get through.
Adding --strict option to 'checkpatch.pl' only also reports something like:
CHECK: multiple assignments should be avoided
#623: FILE: sbc/sbc_primitives.c:381:
+ x[159] = x[31] = pcm[0 + 2];
CHECK: architecture specific defines should be avoided
#475: FILE: sbc/sbc_primitives_mmx.h:31:
+#if defined(__GNUC__) && (defined(__i386__) || defined(__amd64__)) && \
I have learned the following extra bluez coding style requirements over the
standard linux coding style up to now:
1. Space is required for cast operation, ex. '(int) x'
2. Indentation between '#' and 'define' is forbidden
3. Any kind of leading spaces are forbidden
Have I missed something else?
Anyway, does it make sense to introduce a modified variant of 'checkpatch.pl'?
Or probably somebody has done it already, but did not care to share with
everyone else? :)
Best regards,
Siarhei Siamashka
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bluez coding style again
2009-01-17 17:43 Bluez coding style again Siarhei Siamashka
@ 2009-01-18 15:08 ` Marcel Holtmann
2009-01-20 16:48 ` Siarhei Siamashka
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2009-01-18 15:08 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: BlueZ development
Hi Siarhei,
> > commit 1fb5ffff0b704038a90e03860ecce63be2deb988
> > Author: Johan Hedberg <johan.hedberg@nokia.com>
> > Date: Fri Jan 16 20:29:43 2009 +0200
> >
> > Fix indentation to use only tabs
> >
> > --- a/sbc/sbc_tables.h
> > +++ b/sbc/sbc_tables.h
> > @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> > ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> > #define F(x) F_PROTO4(x)
> > static const FIXED_T _sbc_proto_fixed4[40] = {
> > - F(0.00000000E+00), F(5.36548976E-04),
> > + F(0.00000000E+00), F(5.36548976E-04),
> > -F(1.49188357E-03), F(2.73370904E-03),
> > - F(3.83720193E-03), F(3.89205149E-03),
> > - F(1.86581691E-03), F(3.06012286E-03),
> > + F(3.83720193E-03), F(3.89205149E-03),
> > + F(1.86581691E-03), F(3.06012286E-03),
>
> With this latest commit, the vertical alignment of elements in tables gets
> messed up and it becomes harder to see any kind of symmetry in the tables.
> If the spaces are strictly forbidden, I would probably prefer replacing them
> with '+' character.
that would be fine with me.
> Anyway, with this requirement also in effect, it makes harder to use the
> standard 'checkpatch.pl' from linux distribution as you have to watch for
> a lot more stuff which can get through.
>
> Adding --strict option to 'checkpatch.pl' only also reports something like:
>
> CHECK: multiple assignments should be avoided
> #623: FILE: sbc/sbc_primitives.c:381:
> + x[159] = x[31] = pcm[0 + 2];
>
> CHECK: architecture specific defines should be avoided
> #475: FILE: sbc/sbc_primitives_mmx.h:31:
> +#if defined(__GNUC__) && (defined(__i386__) || defined(__amd64__)) && \
>
>
> I have learned the following extra bluez coding style requirements over the
> standard linux coding style up to now:
> 1. Space is required for cast operation, ex. '(int) x'
> 2. Indentation between '#' and 'define' is forbidden
> 3. Any kind of leading spaces are forbidden
>
> Have I missed something else?
>
> Anyway, does it make sense to introduce a modified variant of 'checkpatch.pl'?
> Or probably somebody has done it already, but did not care to share with
> everyone else? :)
Not sure about checkpatch.pl, but the BlueZ coding style is mainly
enforced by Johan and me. In some cases you have to break, but that
should only happen if there is no other choice. 99% of the time you just
can simplify your code. However I happily admit that with codec work
this might not always be true.
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bluez coding style again
2009-01-18 15:08 ` Marcel Holtmann
@ 2009-01-20 16:48 ` Siarhei Siamashka
2009-01-21 11:21 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Siarhei Siamashka @ 2009-01-20 16:48 UTC (permalink / raw)
To: ext Marcel Holtmann; +Cc: BlueZ development
On Sunday 18 January 2009 17:08:12 ext Marcel Holtmann wrote:
> > > --- a/sbc/sbc_tables.h
> > > +++ b/sbc/sbc_tables.h
> > > @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> > > ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> > > #define F(x) F_PROTO4(x)
> > > static const FIXED_T _sbc_proto_fixed4[40] = {
> > > - F(0.00000000E+00), F(5.36548976E-04),
> > > + F(0.00000000E+00), F(5.36548976E-04),
> > > -F(1.49188357E-03), F(2.73370904E-03),
> > > - F(3.83720193E-03), F(3.89205149E-03),
> > > - F(1.86581691E-03), F(3.06012286E-03),
> > > + F(3.83720193E-03), F(3.89205149E-03),
> > > + F(1.86581691E-03), F(3.06012286E-03),
> >
> > With this latest commit, the vertical alignment of elements in tables
> > gets messed up and it becomes harder to see any kind of symmetry in the
> > tables. If the spaces are strictly forbidden, I would probably prefer
> > replacing them with '+' character.
>
> that would be fine with me.
>
> > Anyway, with this requirement also in effect, it makes harder to use the
> > standard 'checkpatch.pl' from linux distribution as you have to watch for
> > a lot more stuff which can get through.
> >
> > Adding --strict option to 'checkpatch.pl' only also reports something
> > like:
> >
> > CHECK: multiple assignments should be avoided
> > #623: FILE: sbc/sbc_primitives.c:381:
> > + x[159] = x[31] = pcm[0 + 2];
> >
> > CHECK: architecture specific defines should be avoided
> > #475: FILE: sbc/sbc_primitives_mmx.h:31:
> > +#if defined(__GNUC__) && (defined(__i386__) || defined(__amd64__)) && \
> >
> >
> > I have learned the following extra bluez coding style requirements over
> > the standard linux coding style up to now:
> > 1. Space is required for cast operation, ex. '(int) x'
> > 2. Indentation between '#' and 'define' is forbidden
> > 3. Any kind of leading spaces are forbidden
> >
> > Have I missed something else?
> >
> > Anyway, does it make sense to introduce a modified variant of
> > 'checkpatch.pl'? Or probably somebody has done it already, but did not
> > care to share with everyone else? :)
>
> Not sure about checkpatch.pl, but the BlueZ coding style is mainly
> enforced by Johan and me. In some cases you have to break, but that
> should only happen if there is no other choice. 99% of the time you just
> can simplify your code. However I happily admit that with codec work
> this might not always be true.
OK, I see. Thanks for the explanation.
Script 'checkpatch.pl' from the linux kernel is quite convenient because it
detects most of the coding style problems automatically and saves some
of the time when preparing a patch.
I'm just desperately trying to do everything in a right way, but still happen
to violate one rule or another occasionally :)
Best regards,
Siarhei Siamashka
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bluez coding style again
2009-01-20 16:48 ` Siarhei Siamashka
@ 2009-01-21 11:21 ` Marcel Holtmann
2009-01-22 15:59 ` Siarhei Siamashka
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2009-01-21 11:21 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: BlueZ development
Hi Siarhei,
> > > > --- a/sbc/sbc_tables.h
> > > > +++ b/sbc/sbc_tables.h
> > > > @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> > > > ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> > > > #define F(x) F_PROTO4(x)
> > > > static const FIXED_T _sbc_proto_fixed4[40] = {
> > > > - F(0.00000000E+00), F(5.36548976E-04),
> > > > + F(0.00000000E+00), F(5.36548976E-04),
> > > > -F(1.49188357E-03), F(2.73370904E-03),
> > > > - F(3.83720193E-03), F(3.89205149E-03),
> > > > - F(1.86581691E-03), F(3.06012286E-03),
> > > > + F(3.83720193E-03), F(3.89205149E-03),
> > > > + F(1.86581691E-03), F(3.06012286E-03),
> > >
> > > With this latest commit, the vertical alignment of elements in tables
> > > gets messed up and it becomes harder to see any kind of symmetry in the
> > > tables. If the spaces are strictly forbidden, I would probably prefer
> > > replacing them with '+' character.
> >
> > that would be fine with me.
> >
> > > Anyway, with this requirement also in effect, it makes harder to use the
> > > standard 'checkpatch.pl' from linux distribution as you have to watch for
> > > a lot more stuff which can get through.
> > >
> > > Adding --strict option to 'checkpatch.pl' only also reports something
> > > like:
> > >
> > > CHECK: multiple assignments should be avoided
> > > #623: FILE: sbc/sbc_primitives.c:381:
> > > + x[159] = x[31] = pcm[0 + 2];
> > >
> > > CHECK: architecture specific defines should be avoided
> > > #475: FILE: sbc/sbc_primitives_mmx.h:31:
> > > +#if defined(__GNUC__) && (defined(__i386__) || defined(__amd64__)) && \
> > >
> > >
> > > I have learned the following extra bluez coding style requirements over
> > > the standard linux coding style up to now:
> > > 1. Space is required for cast operation, ex. '(int) x'
> > > 2. Indentation between '#' and 'define' is forbidden
> > > 3. Any kind of leading spaces are forbidden
> > >
> > > Have I missed something else?
> > >
> > > Anyway, does it make sense to introduce a modified variant of
> > > 'checkpatch.pl'? Or probably somebody has done it already, but did not
> > > care to share with everyone else? :)
> >
> > Not sure about checkpatch.pl, but the BlueZ coding style is mainly
> > enforced by Johan and me. In some cases you have to break, but that
> > should only happen if there is no other choice. 99% of the time you just
> > can simplify your code. However I happily admit that with codec work
> > this might not always be true.
>
> OK, I see. Thanks for the explanation.
>
> Script 'checkpatch.pl' from the linux kernel is quite convenient because it
> detects most of the coding style problems automatically and saves some
> of the time when preparing a patch.
>
> I'm just desperately trying to do everything in a right way, but still happen
> to violate one rule or another occasionally :)
don't worry. Sometimes we have to bend the rules. The highest priority
is readability and make patch management simple for us ;)
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bluez coding style again
2009-01-21 11:21 ` Marcel Holtmann
@ 2009-01-22 15:59 ` Siarhei Siamashka
2009-01-23 1:01 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Siarhei Siamashka @ 2009-01-22 15:59 UTC (permalink / raw)
To: ext Marcel Holtmann; +Cc: BlueZ development
On Wednesday 21 January 2009 13:21:22 ext Marcel Holtmann wrote:
> Hi Siarhei,
>
> > > > > --- a/sbc/sbc_tables.h
> > > > > +++ b/sbc/sbc_tables.h
> > > > > @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> > > > > ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> > > > > #define F(x) F_PROTO4(x)
> > > > > static const FIXED_T _sbc_proto_fixed4[40] = {
> > > > > - F(0.00000000E+00), F(5.36548976E-04),
> > > > > + F(0.00000000E+00), F(5.36548976E-04),
> > > > > -F(1.49188357E-03), F(2.73370904E-03),
> > > > > - F(3.83720193E-03), F(3.89205149E-03),
> > > > > - F(1.86581691E-03), F(3.06012286E-03),
> > > > > + F(3.83720193E-03), F(3.89205149E-03),
> > > > > + F(1.86581691E-03), F(3.06012286E-03),
> > > >
> > > > With this latest commit, the vertical alignment of elements in tables
> > > > gets messed up and it becomes harder to see any kind of symmetry in
> > > > the tables. If the spaces are strictly forbidden, I would probably
> > > > prefer replacing them with '+' character.
> > >
> > > that would be fine with me.
Well, I tried that with the last patch (constants tweaks for audio quality
improvement), but 'checkpatch.pl' tool does not like it very much (it wants
a space between '+' operator and 'F').
[...]
> > I'm just desperately trying to do everything in a right way, but still
> > happen to violate one rule or another occasionally :)
>
> don't worry. Sometimes we have to bend the rules. The highest priority
> is readability and make patch management simple for us ;)
I just returned some spaces as they improve vertical alignment and
readability:
+ F(2.73370904E-03 * C0), F(5.36548976E-04 * C0),
+ -F(1.49188357E-03 * C1), F(0.00000000E+00 * C1),
+ F(3.83720193E-03 * C2), F(1.09137620E-02 * C2),
+ F(3.89205149E-03 * C3), F(3.06012286E-03 * C3),
I hope this should be ok.
--
Best regards,
Siarhei Siamashka
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bluez coding style again
2009-01-22 15:59 ` Siarhei Siamashka
@ 2009-01-23 1:01 ` Marcel Holtmann
0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2009-01-23 1:01 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: BlueZ development
Hi Siarhei,
> > > > > > --- a/sbc/sbc_tables.h
> > > > > > +++ b/sbc/sbc_tables.h
> > > > > > @@ -161,30 +161,30 @@ static const int32_t synmatrix8[16][8] = {
> > > > > > ((FIXED_A) 1 << (sizeof(FIXED_T) * CHAR_BIT - 1)) + 0.5)
> > > > > > #define F(x) F_PROTO4(x)
> > > > > > static const FIXED_T _sbc_proto_fixed4[40] = {
> > > > > > - F(0.00000000E+00), F(5.36548976E-04),
> > > > > > + F(0.00000000E+00), F(5.36548976E-04),
> > > > > > -F(1.49188357E-03), F(2.73370904E-03),
> > > > > > - F(3.83720193E-03), F(3.89205149E-03),
> > > > > > - F(1.86581691E-03), F(3.06012286E-03),
> > > > > > + F(3.83720193E-03), F(3.89205149E-03),
> > > > > > + F(1.86581691E-03), F(3.06012286E-03),
> > > > >
> > > > > With this latest commit, the vertical alignment of elements in tables
> > > > > gets messed up and it becomes harder to see any kind of symmetry in
> > > > > the tables. If the spaces are strictly forbidden, I would probably
> > > > > prefer replacing them with '+' character.
> > > >
> > > > that would be fine with me.
>
> Well, I tried that with the last patch (constants tweaks for audio quality
> improvement), but 'checkpatch.pl' tool does not like it very much (it wants
> a space between '+' operator and 'F').
>
> [...]
>
> > > I'm just desperately trying to do everything in a right way, but still
> > > happen to violate one rule or another occasionally :)
> >
> > don't worry. Sometimes we have to bend the rules. The highest priority
> > is readability and make patch management simple for us ;)
>
> I just returned some spaces as they improve vertical alignment and
> readability:
>
> + F(2.73370904E-03 * C0), F(5.36548976E-04 * C0),
> + -F(1.49188357E-03 * C1), F(0.00000000E+00 * C1),
> + F(3.83720193E-03 * C2), F(1.09137620E-02 * C2),
> + F(3.89205149E-03 * C3), F(3.06012286E-03 * C3),
>
> I hope this should be ok.
fine with me and let Johan know so he doesn't fix it ;)
I am still behind applying your patches, but I will get to it. Just
mention that you really want it applied. Makes it easier for me to go
through the patch logs.
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-01-23 1:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-17 17:43 Bluez coding style again Siarhei Siamashka
2009-01-18 15:08 ` Marcel Holtmann
2009-01-20 16:48 ` Siarhei Siamashka
2009-01-21 11:21 ` Marcel Holtmann
2009-01-22 15:59 ` Siarhei Siamashka
2009-01-23 1:01 ` Marcel Holtmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox