Mark the bit reversal functions as being const as they always return the same
output for any given input.
Signed-Off-By: David Howells <[email protected]>
---
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 */
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 <[email protected]>
> ---
>
> 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
Jeff Garzik <[email protected]> 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
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__ ?
David Howells wrote:
> Jeff Garzik <[email protected]> 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
Jeff Garzik <[email protected]> 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, [email protected]
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."
Andreas Schwab wrote:
> Jeff Garzik <[email protected]> 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
Jeff Garzik <[email protected]> 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
Akinobu Mita <[email protected]> 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
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
Linus Torvalds <[email protected]> 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
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
>> > 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'
--
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