Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Mon, 11 Jul 2005, Horst von Brand wrote:

> > I don't generally disagree with that, I just think that defines are not 
> > part of that list.
> 
> Covered in "bad coding style" and "hard to read code", at least.

Somehow I missed the last lkml debate about where simple defines where a 
problem.

> > Look, it's great that you do reviews, but please keep in mind it's the 
> > author who has to work with code and he has to be primarily happy with, 
> > so you don't have to point out every minor issue.
> 
> Wrong. The author has to work with the code, but there are much more people
> that have to read it now and fix it in the future. It doesn't make sense
> having everybody using their own indentation style, variable naming scheme,
> and ways of defining constants.

I didn't say this, I said "minor issues". Please read more carefully.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Horst von Brand]

Roman Zippel <[EMAIL PROTECTED]> wrote:
> On Sun, 10 Jul 2005, Pekka Enberg wrote:

[...]

> > Would you please be so kind to define your criteria for things that
> > "need fixing" so we could see if can reach some sort of an agreement on
> > this. My list is roughly as follows:
> > 
> >   - Erroneous use of kernel API
> >   - Bad coding style
> >   - Layering violations
> >   - Duplicate code
> >   - Hard to read code

> I don't generally disagree with that, I just think that defines are not 
> part of that list.

Covered in "bad coding style" and "hard to read code", at least.

> Look, it's great that you do reviews, but please keep in mind it's the 
> author who has to work with code and he has to be primarily happy with, 
> so you don't have to point out every minor issue.

Wrong. The author has to work with the code, but there are much more people
that have to read it now and fix it in the future. It doesn't make sense
having everybody using their own indentation style, variable naming scheme,
and ways of defining constants. For better or worse, #define /is/ the
standard idiom (in kernel and elsewhere) to define constants in C. See also
, particularly
comandment 8. And consider also the /spirit/ of Documentation/CodingStyle.

> Although it also differs between core and driver code, we don't have to be 
> that strict with driver code as longs as it "looks" ok and is otherwise 
> correct. The requirements for core kernel code are higher, but even here 
> defines are a well accepted language construct.

I'd rather ask for the higher requirements of authors of new code... not
demand, just ask.
-- 
Dr. Horst H. von Brand   User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria  +56 32 654239
Casilla 110-V, Valparaiso, ChileFax:  +56 32 797513
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Horst von Brand]

Vojtech Pavlik <[EMAIL PROTECTED]> wrote:
> On Sun, Jul 10, 2005 at 09:21:42PM +0300, Pekka Enberg wrote:
> > Hmm. So we disagree on that issue as well. I think the point of review
> > is to improve code and help others conform with the existing coding
> > style which is why I find it strange that you're suggesting me to limit
> > my comments to a subset you agree with.
> > 
> > Would you please be so kind to define your criteria for things that
> > "need fixing" so we could see if can reach some sort of an agreement on
> > this. My list is roughly as follows:
> > 
> >   - Erroneous use of kernel API
> >   - Bad coding style
> >   - Layering violations
> >   - Duplicate code
> >   - Hard to read code

> The reason people post their patches for review is to get good feedback
> on them. The problems you list above are mostly nitpicks. They must be
> fixed before inclusion of the patch, but only make sense to start fixing
> once the patch does a reasonable change.

If the coding style is an obstacle to understanding, it has to be fixed if
there is going to be any kind of review. Besides, nitpicks being simple to
address, they could as well be fixed while at it. Perhaps that way the
author learns to do it right, and less nitpicks are left in later additions
and fixes.
-- 
Dr. Horst H. von Brand   User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria  +56 32 654239
Casilla 110-V, Valparaiso, ChileFax:  +56 32 797513
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka J Enberg]

Hi Roman, 


Roman Zippel writes:
I don't generally disagree with that, I just think that defines are not 
part of that list.


They're in Documentation/CodingStyle (see Chapter 11). 


Roman Zippel writes:
Look, it's great that you do reviews, but please keep in mind it's the 
author who has to work with code and he has to be primarily happy with, 
so you don't have to point out every minor issue.


I completely agree with you that the author must be happy with the code. I 
also think Vojtech has a good point about reviewing for the most important 
things first and worrying about nitpicks later. 

I'll try to find a better balance for reviews. Thanks Roman. 

 Pekka 


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pavel Machek]

Hi!

> >>enums in C are (de?)promoted to integral types under most conditions, so 
> >>the type-checking is useless.
> >
> >
> >It's a warning in gcc afaik and spare should complain as well.
> 
> Check again.

Check sparse with -Wbitwise and enum properly marked as bitwise...

Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Vojtech Pavlik]

On Sun, Jul 10, 2005 at 09:21:42PM +0300, Pekka Enberg wrote:

> Hmm. So we disagree on that issue as well. I think the point of review
> is to improve code and help others conform with the existing coding
> style which is why I find it strange that you're suggesting me to limit
> my comments to a subset you agree with.
> 
> Would you please be so kind to define your criteria for things that
> "need fixing" so we could see if can reach some sort of an agreement on
> this. My list is roughly as follows:
> 
>   - Erroneous use of kernel API
>   - Bad coding style
>   - Layering violations
>   - Duplicate code
>   - Hard to read code
 
The reason people post their patches for review is to get good feedback
on them. The problems you list above are mostly nitpicks. They must be
fixed before inclusion of the patch, but only make sense to start fixing
once the patch does a reasonable change.

Often patches have deeper problems (like "this won't ever work", "there
is a nice race hidden in there", "why do we need this part at all"), and
spotting those is much more valuable for both the sumbitter and the
progress of development.

Obviously, it's much harder to do that than to comment on a misplaced
brace.

It's an utter waste of effort to force a first time patch author to fix
all the style issues in his patch, just to see it rejected by the
maintainer because it is fundamentally wrong later.

Just something to consider.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Sun, 10 Jul 2005, Pekka Enberg wrote:

> > The point of a review is to comment on things that _need_ fixing. Less 
> > experienced hackers take this a requirement for their drivers to be 
> > included.
> 
> Hmm. So we disagree on that issue as well. I think the point of review
> is to improve code and help others conform with the existing coding
> style which is why I find it strange that you're suggesting me to limit
> my comments to a subset you agree with.
> 
> Would you please be so kind to define your criteria for things that
> "need fixing" so we could see if can reach some sort of an agreement on
> this. My list is roughly as follows:
> 
>   - Erroneous use of kernel API
>   - Bad coding style
>   - Layering violations
>   - Duplicate code
>   - Hard to read code

I don't generally disagree with that, I just think that defines are not 
part of that list.
Look, it's great that you do reviews, but please keep in mind it's the 
author who has to work with code and he has to be primarily happy with, 
so you don't have to point out every minor issue.
Although it also differs between core and driver code, we don't have to be 
that strict with driver code as longs as it "looks" ok and is otherwise 
correct. The requirements for core kernel code are higher, but even here 
defines are a well accepted language construct.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[randy_dunlap]

On Sun, 10 Jul 2005 21:21:42 +0300 Pekka Enberg wrote:

| Hi Roman,
| 
| At some point in time, I wrote:
| > > Roman, it is not as if I get to decide for the patch submitters. I
| > > comment on any issues _I_ have with the patch and the authors fix
| > > whatever they want (or what the maintainers ask for).
| 
| On Fri, 2005-07-08 at 21:59 +0200, Roman Zippel wrote:
| > The point of a review is to comment on things that _need_ fixing. Less 
| > experienced hackers take this a requirement for their drivers to be 
| > included.
| 
| Hmm. So we disagree on that issue as well. I think the point of review
| is to improve code and help others conform with the existing coding
| style which is why I find it strange that you're suggesting me to limit
| my comments to a subset you agree with.
| 
| Would you please be so kind to define your criteria for things that
| "need fixing" so we could see if can reach some sort of an agreement on
| this. My list is roughly as follows:
| 
|   - Erroneous use of kernel API
|   - Bad coding style
|   - Layering violations
|   - Duplicate code
|   - Hard to read code

Those are all good points IMO, but there is little (or no)
concensus on "bad coding style" or "hard to read code".

Maybe it would make more sense to make some suggested changes to
Documentation/CodingStyle (in the form of a patch) and see what
kinds of reactions (support) it gets.

---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka Enberg]

Hi Roman,

At some point in time, I wrote:
> > Roman, it is not as if I get to decide for the patch submitters. I
> > comment on any issues _I_ have with the patch and the authors fix
> > whatever they want (or what the maintainers ask for).

On Fri, 2005-07-08 at 21:59 +0200, Roman Zippel wrote:
> The point of a review is to comment on things that _need_ fixing. Less 
> experienced hackers take this a requirement for their drivers to be 
> included.

Hmm. So we disagree on that issue as well. I think the point of review
is to improve code and help others conform with the existing coding
style which is why I find it strange that you're suggesting me to limit
my comments to a subset you agree with.

Would you please be so kind to define your criteria for things that
"need fixing" so we could see if can reach some sort of an agreement on
this. My list is roughly as follows:

  - Erroneous use of kernel API
  - Bad coding style
  - Layering violations
  - Duplicate code
  - Hard to read code

Pekka

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Denis Vlasenko]

On Friday 08 July 2005 19:57, Roman Zippel wrote:
> Hi,
> 
> On Fri, 8 Jul 2005, Bryan Henderson wrote:
> 
> > I wasn't aware anyone preferred defines to enums for declaring enumerated 
> > data types.
> 
> If it's really enumerated data types, that's fine, but this example was 
> about bitfield masks.
> 
> > Isn't the only argument for defines, "that's what I'm used to."?
> 
> defines are not just used for constants and there is _nothing_ wrong with 
> using defines for constants.

Apart from the wart that defines know zilch about scopes.
Thus completely fine looking code will give you atrociously
obscure compiler message. Real world example for userspace:

# cat t.c
#include 
#include 

void f() {
char errno[] = "hello world";
puts(errno);
}

# gcc -c t.c
t.c: In function `f':
t.c:5: error: function `__errno_location' is initialized like a variable
t.c:5: error: conflicting types for '__errno_location'
/usr/include/bits/errno.h:38: error: previous declaration of '__errno_location' 
was here
--
vda

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka Enberg]

Hi,

On Fri, 2005-07-08 at 21:11 +0200, Roman Zippel wrote:
> So it basically comes down to personal preference, if the original uses 
> defines and it works fine, I don't really see a good enough reason to 
> change it to enums, so please leave the decision to author.

(And I don't see a good enough reason to use #defines when you don't
 absolutely have to. This is what we disagree on.)

Roman, it is not as if I get to decide for the patch submitters. I
comment on any issues _I_ have with the patch and the authors fix
whatever they want (or what the maintainers ask for).

As I disagree with the part about enums being a personal preference, I
will continue to comment on them in the future. If patch authors wish to
ignore them (or any of my comments for that matter), that's ok with me.

P.S. Working code is not enough for the kernel. It must be maintainable
as well.

Pekka

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

>I don't see how the following is tortured: 
>
>enum {
>   PNODE_MEMBER_VFS  = 0x01,
>   PNODE_SLAVE_VFS   = 0x02
>}; 

Only because it's using a facility that's supposed to be for enumerated 
types for something that isn't.  If it were a true enumerated type, the 
codes for the enumerations (0x01, 0x02) would be quite arbitrary, whereas 
here they must fundamentally be integers whose pure binary cipher has 
exactly one 1 bit (because, as I understand it, these are used as bitmasks 
somewhere).

I can see that this paradigm has practical advantages over using macros 
(or a middle ground - integer constants), but only as a byproduct of what 
the construct is really for.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Mike Waychison]

Wichert Akkerman wrote:

Previously Mike Waychison wrote:

enums in C are (de?)promoted to integral types under most conditions, so 
the type-checking is useless.



It's a warning in gcc afaik and spare should complain as well.



Check again.

You must be thinking of another language.

Show me how you can get warnings out of this:

enum foo { FOO, };


static enum foo doit(enum foo bar) {
int i = bar;
return i;
}


int main(void)
{
int i;
i = doit(0);
return 0;
}

Mike Waychison
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Ram]

On Fri, 2005-07-08 at 12:11, Roman Zippel wrote:
> Hi,
> 
> On Fri, 8 Jul 2005, Pekka J Enberg wrote:
> 
> > I don't see how the following is tortured: 
> > enum {
> >   PNODE_MEMBER_VFS  = 0x01,
> >   PNODE_SLAVE_VFS   = 0x02
> > }; 
> > In fact, I think it is more natural. An almost identical example even 
> > appears
> > in K&R. 
> 
> So it basically comes down to personal preference, if the original uses 
> defines and it works fine, I don't really see a good enough reason to 
> change it to enums, so please leave the decision to author.

Ok. I will change to enums whereever I define new categories of
#defines. And leave the #defines for already existing category as is.

RP


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka Enberg wrote:

> On Fri, 2005-07-08 at 21:11 +0200, Roman Zippel wrote:
> > So it basically comes down to personal preference, if the original uses 
> > defines and it works fine, I don't really see a good enough reason to 
> > change it to enums, so please leave the decision to author.
> 
> (And I don't see a good enough reason to use #defines when you don't
>  absolutely have to. This is what we disagree on.)

"use" != "change".
If an author already uses defines, that's fine and in most cases there is 
no reason to change it.

> Roman, it is not as if I get to decide for the patch submitters. I
> comment on any issues _I_ have with the patch and the authors fix
> whatever they want (or what the maintainers ask for).

The point of a review is to comment on things that _need_ fixing. Less 
experienced hackers take this a requirement for their drivers to be 
included.

> P.S. Working code is not enough for the kernel. It must be maintainable
> as well.

defines are perfectly maintainable.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> I don't see how the following is tortured: 
> enum {
>   PNODE_MEMBER_VFS  = 0x01,
>   PNODE_SLAVE_VFS   = 0x02
> }; 
> In fact, I think it is more natural. An almost identical example even appears
> in K&R. 

So it basically comes down to personal preference, if the original uses 
defines and it works fine, I don't really see a good enough reason to 
change it to enums, so please leave the decision to author.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka J Enberg]

Roman Zippel writes:
> If it's really enumerated data types, that's fine, but this example was 
> about bitfield masks.


Bryan Henderson writes:
Ah.  In that case, enum is a pretty tortured way to declare it, though it 
does have the practical advantages over define that have been mentioned 
because the syntax is more rigorous.


I think the bit we're talking about is this: 


Index: 2.6.12/fs/pnode.c
===
--- /dev/null
+++ 2.6.12/fs/pnode.c
@@ -0,0 +1,362 @@
+
+#define PNODE_MEMBER_VFS  0x01
+#define PNODE_SLAVE_VFS   0x02


I don't see how the following is tortured: 


enum {
  PNODE_MEMBER_VFS  = 0x01,
  PNODE_SLAVE_VFS   = 0x02
}; 

In fact, I think it is more natural. An almost identical example even 
appears in K&R. 

  Pekka 


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Wichert Akkerman]

Previously Mike Waychison wrote:
> enums in C are (de?)promoted to integral types under most conditions, so 
> the type-checking is useless.

It's a warning in gcc afaik and spare should complain as well.

Wichert.

-- 
Wichert Akkerman <[EMAIL PROTECTED]>It is simple to make things.
http://www.wiggy.net/   It is hard to make things simple.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Mike Waychison]

Wichert Akkerman wrote:

Previously Bryan Henderson wrote:

Two advantages of the enum declaration that haven't been mentioned yet, 
that help me significantly:



There is another one: with enums the compiler checks types for you.



enums in C are (de?)promoted to integral types under most conditions, so 
the type-checking is useless.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Wichert Akkerman]

Previously Bryan Henderson wrote:
> Two advantages of the enum declaration that haven't been mentioned yet, 
> that help me significantly:

There is another one: with enums the compiler checks types for you.

Wichert.

-- 
Wichert Akkerman <[EMAIL PROTECTED]>It is simple to make things.
http://www.wiggy.net/   It is hard to make things simple.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

>If it's really enumerated data types, that's fine, but this example was 
>about bitfield masks.

Ah.  In that case, enum is a pretty tortured way to declare it, though it 
does have the practical advantages over define that have been mentioned 
because the syntax is more rigorous.

The proper way to do bitfield masks is usually C bit field declarations, 
but I understand that tradition works even more strongly against using 
those than against using enum to declare enumerated types.

>there is _nothing_ wrong with using defines for constants.

I disagree with that; I find practical and, more importantly, 
philosophical reasons not to use defines for constants.  I'm sure you've 
heard the arguments; I just didn't want to let that statement go 
uncontested.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Bryan Henderson wrote:

> I wasn't aware anyone preferred defines to enums for declaring enumerated 
> data types.

If it's really enumerated data types, that's fine, but this example was 
about bitfield masks.

> Isn't the only argument for defines, "that's what I'm used to."?

defines are not just used for constants and there is _nothing_ wrong with 
using defines for constants.

> The macro language is one the most hated parts of the C language; it makes 
> sense to try to avoid it as a general rule.

Nevertheless it's part of the language, it's used all over the kernel and 
suddenly starting to mix different types of definitions, makes things 
only worse. I prefer consistency here over any minor advantages enums 
might have.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

I wasn't aware anyone preferred defines to enums for declaring enumerated 
data types.  The practical advantages of enums are slight, but as far as I 
know, the practical advantages of defines are zero.  Isn't the only 
argument for defines, "that's what I'm used to."?

Two advantages of the enum declaration that haven't been mentioned yet, 
that help me significantly:

- if you have a typo in a define, it can be really hard to interpret the 
compiler error messages.  The same typo in an enum gets a pointed error 
message referring to the line that has the typo.

- Gcc warns you if a switch statement doesn't handle every case.  I often 
add an enumeration and Gcc lets me know where I forgot to consider it.

The macro language is one the most hated parts of the C language; it makes 
sense to try to avoid it as a general rule.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html