Mark bitrevX() functions as const

Mark bitrevX() functions as const

[David Howells]

Mark the bit reversal functions as being const as they always return the same
output for any given input.

Signed-Off-By: David Howells <dhowells@redhat.com>
---

 include/linux/bitrev.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
index 05e540d..032056b 100644
--- a/include/linux/bitrev.h
+++ b/include/linux/bitrev.h
@@ -5,11 +5,11 @@ #include <linux/types.h>
 
 extern u8 const byte_rev_table[256];
 
-static inline u8 bitrev8(u8 byte)
+static inline __attribute__((const)) u8 bitrev8(u8 byte)
 {
 	return byte_rev_table[byte];
 }
 
-extern u32 bitrev32(u32 in);
+extern __attribute__((const)) u32 bitrev32(u32 in);
 
 #endif /* _LINUX_BITREV_H */

Re: Mark bitrevX() functions as const

[Jeff Garzik]

David Howells wrote:
> Mark the bit reversal functions as being const as they always return the same
> output for any given input.
> 
> Signed-Off-By: David Howells <dhowells@redhat.com>
> ---
> 
>  include/linux/bitrev.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
> index 05e540d..032056b 100644
> --- a/include/linux/bitrev.h
> +++ b/include/linux/bitrev.h
> @@ -5,11 +5,11 @@ #include <linux/types.h>
>  
>  extern u8 const byte_rev_table[256];
>  
> -static inline u8 bitrev8(u8 byte)
> +static inline __attribute__((const)) u8 bitrev8(u8 byte)
>  {
>  	return byte_rev_table[byte];
>  }
>  
> -extern u32 bitrev32(u32 in);
> +extern __attribute__((const)) u32 bitrev32(u32 in);


Comments:

* overall, I agree with this type of change.  several Linux lib 
functions could use this sort of annotation.

* I question its usefulness on static [inline] functions, because the 
compiler should be able to figure out side effects.  have you examined 
before-and-after asm to see if the code generation changes for the 
inlined area?

* naked __attribute__ is ugly.  define something short and memorable in 
include/linux/compiler.h.

* another annotation to consider is C99 keyword 'restrict'.

	Jeff

Re: Mark bitrevX() functions as const

[Andreas Schwab]

Jeff Garzik <jeff@garzik.org> writes:

> * another annotation to consider is C99 keyword 'restrict'.

This is useless as long as we compile with -fno-strict-aliasing (and I
don't think this will ever change).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Mark bitrevX() functions as const

[Jeff Garzik]

Andreas Schwab wrote:
> Jeff Garzik <jeff@garzik.org> writes:
> 
>> * another annotation to consider is C99 keyword 'restrict'.
> 
> This is useless as long as we compile with -fno-strict-aliasing (and I
> don't think this will ever change).

Yes, just as useless as __attribute__((bitwise))... :)

	Jeff

Re: Mark bitrevX() functions as const

[David Howells]

Jeff Garzik <jeff@garzik.org> wrote:

> * overall, I agree with this type of change.  several Linux lib functions
> could use this sort of annotation.

Yes.  I just happened to notice bitrev.c at the end of my git pull and wondered
if it was what it sounded like...

> * I question its usefulness on static [inline] functions, because the compiler
> should be able to figure out side effects.  have you examined before-and-after
> asm to see if the code generation changes for the inlined area?

It doesn't actually make any difference, but I think such functions should be
so marked anyway: it gives both the code writer and the compiler more
information (though they're both free to ignore it if they like).

> * naked __attribute__ is ugly.  define something short and memorable in
> include/linux/compiler.h.

I'm not sure that's a good idea.  You have to be careful not to cause confusion
with ordinary "const".

> * another annotation to consider is C99 keyword 'restrict'.

Indeed, though I presume you don't mean in this particular case...

David

Re: Mark bitrevX() functions as const

[Jeff Garzik]

David Howells wrote:
> Jeff Garzik <jeff@garzik.org> wrote:
>> * naked __attribute__ is ugly.  define something short and memorable in
>> include/linux/compiler.h.
> 
> I'm not sure that's a good idea.  You have to be careful not to cause confusion
> with ordinary "const".

It's all in the naming.  You could call it 'purefunc' or somesuch.

__attribute__ is very very ugly, an hinders a quick scan of the function 
prototype, particularly if it has a boatload of other attributes.


>> * another annotation to consider is C99 keyword 'restrict'.
> 
> Indeed, though I presume you don't mean in this particular case...

Correct.

	Jeff

Re: Mark bitrevX() functions as const

[David Howells]

Jeff Garzik <jeff@garzik.org> wrote:

> > I'm not sure that's a good idea.  You have to be careful not to cause
> > confusion with ordinary "const".
> 
> It's all in the naming.  You could call it 'purefunc' or somesuch.

No, not "pure".  That's something else.

> __attribute__ is very very ugly, an hinders a quick scan of the function
> prototype, particularly if it has a boatload of other attributes.

Maybe you should do:

	extern __attibute__((x, y, z))
	void function_prototype(...);

Then it doesn't hinder it anywhere near as much as, say:

	extern void __fastcall function_prototype(...);

Besides, emacs lights up __attribute__'s in funky colours to make them easier
to look past:-)

David

Re: Mark bitrevX() functions as const

[Akinobu Mita]

On Mon, Dec 11, 2006 at 12:35:36PM +0000, David Howells wrote:
> diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
> index 05e540d..032056b 100644
> --- a/include/linux/bitrev.h
> +++ b/include/linux/bitrev.h
> @@ -5,11 +5,11 @@ #include <linux/types.h>
>  
>  extern u8 const byte_rev_table[256];
>  
> -static inline u8 bitrev8(u8 byte)
> +static inline __attribute__((const)) u8 bitrev8(u8 byte)
>  {
>  	return byte_rev_table[byte];
>  }
>  
> -extern u32 bitrev32(u32 in);
> +extern __attribute__((const)) u32 bitrev32(u32 in);
>  
>  #endif /* _LINUX_BITREV_H */

__attribute_pure__ ?

Re: Mark bitrevX() functions as const

[David Howells]

Akinobu Mita <akinobu.mita@gmail.com> wrote:

> __attribute_pure__ ?

I'm not sure "pure" is better than const in this case.  Although it *does* look
at a global variable (byte_rev_table), that variable is constant.  In effect,
the functions output does only depend on its input.  The R/O data it requires
is no different to its out of line code.  You'd have to consult a compiler
wrangler to be sure, though.

David

Re: Mark bitrevX() functions as const

[Linus Torvalds]

On Mon, 11 Dec 2006, David Howells wrote:
> 
> Mark the bit reversal functions as being const as they always return the same
> output for any given input.

Well, we should mark the argument const too, no?

Does anythign actually improve from this? Also, we should actually use 
"__attribute_const__" instead (which works with other compilers), not the 
gcc'ism. That "__attribute__((const))" thing is a horrible syntax anyway 
(and has apparently slipped into <linux/log2.h> too - Damn.

		Linus

Re: Mark bitrevX() functions as const

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:

> > Mark the bit reversal functions as being const as they always return the
> > same output for any given input.
> 
> Well, we should mark the argument const too, no?

The argument is just an integer; I'm not sure that marking it const actually
achieves anything, except to tell the function that it can't modify it - and
since it's effectively a copy, where's the fun in that.

> Does anythign actually improve from this? Also, we should actually use 
> "__attribute_const__" instead (which works with other compilers), not the 
> gcc'ism. That "__attribute__((const))" thing is a horrible syntax anyway 
> (and has apparently slipped into <linux/log2.h> too - Damn.

Ah.  I thought that was just for supporting old versions of gcc.  I didn't
realise it was for handling strange compilers.

David

Re: Mark bitrevX() functions as const

[Linus Torvalds]

On Mon, 11 Dec 2006, David Howells wrote:
> 
> Ah.  I thought that was just for supporting old versions of gcc.  I didn't
> realise it was for handling strange compilers.

I'm not sure how much (if at all) the Intel compiler is actually used, and 
for all I know it may even support __attribute__((__const__)) these days, 
but I like the notion of allowing us to support other compilers, so the 
infrastructure is all set up for that.

The main <linux/compiler.h> thing includes various per-compiler headers, 
and then defaults some things to be empty if the compiler-specific header 
doesn't have its own #define for it. So it's actually set up to try to 
help more than just gcc or the Intel compiler, although nobody has done 
anything else afaik.

I think all versions of gcc support the __attribute__((const)) thing (and 
indeed, it's in the "generic" gcc header file, not the per-gcc-version 
one).

		Linus

Re: Mark bitrevX() functions as const

[Jan Engelhardt]

>> > Mark the bit reversal functions as being const as they always return the
>> > same output for any given input.
>> 
>> Well, we should mark the argument const too, no?
>
>The argument is just an integer; I'm not sure that marking it const actually
>achieves anything, except to tell the function that it can't modify it - and
>since it's effectively a copy, where's the fun in that.

I can just second this. What should be marked const is [1]the things 
pointed to, not [2]the local copy of a function argument.

This[2] is what I believe almost every other software project does, 
though they often fail at [1]. Or have you seen Glibc trying to pull a
int strtoul(const char *const nptr, char **const endptr, const int 
base)? It just makes the prototypes and headers longer without having 
too much benefit. And maybe the code author may even want to reuse the 
args directly as walking pointers or countdown integers, for example.


	-`J'
-- 

Re: Mark bitrevX() functions as const

[Bernd Petrovitsch]

On Mon, 2006-12-11 at 18:35 +0100, Jan Engelhardt wrote:
[...]
> I can just second this. What should be marked const is [1]the things 
> pointed to, not [2]the local copy of a function argument.
> 
> This[2] is what I believe almost every other software project does, 

Yes, also for the reason to educate people to actually use "const" as
much as possible - if only to make it for humans clear what may change
somewhere and what not.

> though they often fail at [1]. Or have you seen Glibc trying to pull a
> int strtoul(const char *const nptr, char **const endptr, const int 
> base)? It just makes the prototypes and headers longer without having 

glibc functions like strtoul() are an extremely bad example for this
because there are standards out there which the define the function
signature - so it is often not really the choice of some glibc
developer/maintainer/project lead.
Or you have very old implementations like e.g. the RPC/XDR library which
simply ignore the "const" keyword.

> too much benefit. And maybe the code author may even want to reuse the 
> args directly as walking pointers or countdown integers, for example.

And that is the other problem of such functions and C as such: One wants
the "const char *" in the argument list (if possible) since it allows to
pass "const char *" and "char *". The return value should similarly be
"char *" because then you can use it in the above mentioned way for
"const char *" and "char *".
Alas that gives you a chance to "cast" "const char *" to "char *" and
not even trigger a compiler warning (as opposed a real type cast). Of
course this can be handled/fixed by review but it takes people to
actually do this.
The only sane solution is to get out the same const-ness as passed in -
but this is syntactically not possible in plain simple C.

And the above paragraph is not arguing to remove the keyword "const".

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services