HACKING: remove bogus restrictions
PATCH 1/1 HACKING: remove bogus restrictions
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.
These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit.
--- HACKING | 4 ---- 1 file changed, 4 deletions(-)
HACKING +0 -4
@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
Signature · 1 line
MST
Quoted 6 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.These rules are also widely violated in our codebase,
To add to that, they are even contradicted in HACKING itself which suggests using ram_addr_t for RAM offsets (_t suffix).
Quoted 25 lines
and it does not make sense to rework it all, apparently for no benefit.
Signed-off-by Michael S. Tsirkin <[email protected]>--- HACKING | 4 ---- 1 file changed, 4 deletions(-)HACKING +0
-4@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalignSignature · 1 line
MST
HACKING it points to, or it is aliased to another pointer that is. · line 74
Quoted 8 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.-2.4. Reserved namespaces in C and POSIX
HACKING +0
-2-Underscore capital, double underscore, and underscore 't' suffixes should be -avoided.
I think this is just a missing "prefixes". C99 7.1.3
reserves underscore capital and double underscore prefixes.
POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html)
I would suggest that the section is reworded to:
# Underscore capital and double underscore prefixes are reserved
# by the C standard. Underscore 't' suffixes are reserved by POSIX. # They should be avoided in QEMU code.
-- PMM
Quoted 6 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.
Quoted 6 lines
HACKING it points to, or it is aliased to another pointer that is. · line 74
Quoted 3 lines
-2.4. Reserved namespaces in C and POSIX
HACKING +0
-2-Underscore capital, double underscore, and underscore 't' suffixes should be -avoided.I think this is just a missing "prefixes". C99 7.1.3 reserves underscore capital and double underscore prefixes.
This is taking it out of context - reserved means different things in different parts of the spec.
As far as I can see, 7.1.3 talks about what is permissible in headers:
naturally when a libc implementation adds stuff in a header it would want to avoid breaking applications including this header. But it does not talk about what is permissible in a legal program - to avoid conflicts, you just need to use variables with reasonable names like kvmXXX qemuXXX or virtioXXX.
So it does not look like this is a reason to not use __kvm_pv_eoi_disabled.
Chances that a libc header predefines this name are zero.
Quoted 2 lines
POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html)
Same thing really.
So this says posix implementations can add types ending with _t in any header. It does not prohibit applications from using such names in a reasonable manner, and it does not look like this is a reason to not use qemu_foo_bar_t.
Quoted 7 lines
I would suggest that the section is reworded to:
# Underscore capital and double underscore prefixes are reserved
# by the C standard. Underscore 't' suffixes are reserved by POSIX. # They should be avoided in QEMU code.-- PMM
This might be nice from language purism point of view but ignores the fact that QEMU already uses lots of these. Specifically a _t type is even mentioned in HACKING itself.
So the benefit to excluding this in new code is marginal.
Signature · 1 line
MST
Quoted 7 lines · Michael S. Tsirkin
Quoted 2 lines · Peter Maydell
C99 7.1.3
reserves underscore capital and double underscore prefixes.This is taking it out of context - reserved means different things in different parts of the spec.
Quoted 6 lines
As far as I can see, 7.1.3 talks about what is permissible in headers:
naturally when a libc implementation adds stuff in a header it would want to avoid breaking applications including this header. But it does not talk about what is permissible in a legal program - to avoid conflicts, you just need to use variables with reasonable names like kvmXXX qemuXXX or virtioXXX.
7.1.3 para 2 says "If the program declares or defines an identifier in a context in which it is reserved [...] the behavior is undefined." The rationale for 7.1.3 says that the underscore-cap and double underscore identifiers are "reserved for the implementor" and that "part of the name space of internal identifiers beginning with underscore is available to the user" which implies that the rest of that namespace is *not* available to the user.
I think that's fairly clear that we can't use these identifiers without entering the realm of undefined behavior.
-- PMM
Quoted 23 lines · Peter Maydell
Quoted 7 lines · Michael S. Tsirkin
Quoted 2 lines · Peter Maydell
C99 7.1.3
reserves underscore capital and double underscore prefixes.This is taking it out of context - reserved means different things in different parts of the spec.
Quoted 6 lines
As far as I can see, 7.1.3 talks about what is permissible in headers:
naturally when a libc implementation adds stuff in a header it would want to avoid breaking applications including this header. But it does not talk about what is permissible in a legal program - to avoid conflicts, you just need to use variables with reasonable names like kvmXXX qemuXXX or virtioXXX.7.1.3 para 2 says "If the program declares or defines an identifier in a context in which it is reserved [...] the behavior is undefined." The rationale for 7.1.3 says that the underscore-cap and double underscore identifiers are "reserved for the implementor" and that "part of the name space of internal identifiers beginning with underscore is available to the user" which implies that the rest of that namespace is *not* available to the user.
Quoted 3 lines
I think that's fairly clear that we can't use these identifiers without entering the realm of undefined behavior.
I'll have to re-read that, I missed it.
What about _t in POSIX? That seems fairly safe if name is long and qemu specific enough.
Signature · 1 line
MST
Quoted 2 lines · Michael S. Tsirkin
What about _t in POSIX? That seems fairly safe if name is long and qemu specific enough.
Depends what you mean by "safe". The spec says "don't use this"; it isn't any different to the __ and _[A-Z] prohibitions in that respect.
Other posix namespace landgrabs you may not have expected:
* ctype.h reserves "is[a-z]" and to[a-z]" prefixes
* string.h takes "str[a-z]" and "mem[a-z]" prefixes
(and qemu-common.h includes both string.h and ctype.h so this effectively applies to all of qemu).
I'm in two minds about these, because on the one hand they're reserved but on the other hand abiding by the rules makes things uglier (whereas "use one underscore not two" doesn't produce an uglier name IMHO).
(posix also if I'm reading it correctly reserves all of 'prefix underscore', which is annoying.)
-- PMM
Quoted 7 lines · Peter Maydell
Quoted 2 lines · Michael S. Tsirkin
What about _t in POSIX? That seems fairly safe if name is long and qemu specific enough.
Depends what you mean by "safe". The spec says "don't use this"; it isn't any different to the __ and _[A-Z] prohibitions in that respect.
It's different: unlike C compiler POSIX can not mangle names in an application. So it will not internally create qemu_foo_bar_t from qemu_foo_bar, and thus qemu_foo_bar_t is very unlikely to create conflicts.
OTOH compiler people do mysterious and strange things to the point where you do not want to mess with them if you can.
Quoted 12 lines
Other posix namespace landgrabs you may not have expected:
*ctype.hreserves "is[a-z]" and to[a-z]" prefixes
*string.htakes "str[a-z]" and "mem[a-z]" prefixes(and
qemu-common.hincludes bothstring.handctype.hso this effectively applies to all of qemu).I'm in two minds about these, because on the one hand they're reserved but on the other hand abiding by the rules makes things uglier (whereas "use one underscore not two" doesn't produce an uglier name IMHO).
Quoted 4 lines
(posix also if I'm reading it correctly reserves all of 'prefix underscore', which is annoying.)
-- PMM
Exactly my points.
Signature · 1 line
MST
Quoted 14 lines · Michael S. Tsirkin
Quoted 6 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.Quoted 6 lines
HACKING it points to, or it is aliased to another pointer that is. · line 74
Quoted 3 lines
-2.4. Reserved namespaces in C and POSIX
HACKING +0
-2-Underscore capital, double underscore, and underscore 't' suffixes should be -avoided.I think this is just a missing "prefixes". C99 7.1.3 reserves underscore capital and double underscore prefixes.
Quoted 2 lines
This is taking it out of context - reserved means different things in different parts of the spec.
Reserved means - reserved for implementation to use as it sees fit, for instance - 6.2.5
28) Implementation-defined keywords shall have the form of an identifier reserved for any use as described in 7.1.3.
Future versions of C might define something in the form __xxx or _[A-Z]xxx precisely because those are invalid for user to define.. (_Imaginary, _Bool, whatever)
7.1.3#2
Quoted 2 lines
As far as I can see, 7.1.3 talks about what is permissible in headers:
And you are wrong.
Quoted 36 lines
naturally when a libc implementation adds stuff in a header it would want to avoid breaking applications including this header. But it does not talk about what is permissible in a legal program - to avoid conflicts, you just need to use variables with reasonable names like kvmXXX qemuXXX or virtioXXX.
So it does not look like this is a reason to not use
__kvm_pv_eoi_disabled.Chances that a libc header predefines this name are zero.
Quoted 2 lines
POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html)
Same thing really.
So this says posix implementations can add types ending with _t in any header. It does not prohibit applications from using such names in a reasonable manner, and it does not look like this is a reason to not useqemu_foo_bar_t.Quoted 7 lines
I would suggest that the section is reworded to:
# Underscore capital and double underscore prefixes are reserved
# by the C standard. Underscore 't' suffixes are reserved by POSIX. # They should be avoided in QEMU code.-- PMM
This might be nice from language purism point of view but ignores the fact that QEMU already uses lots of these. Specifically a _t type is even mentioned in HACKING itself.
So the benefit to excluding this in new code is marginal.
Signature · 1 line
mailto:[email protected]
Quoted 8 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit.
NACK. The benefit is improved standards compliance. One day we could find QEMU being ported to environment which conflicts with these symbols.
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Quoted 23 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- HACKING | 4 ---- 1 file changed, 4 deletions(-)HACKING +0
-5@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign --MST
Quoted 6 lines · Blue Swirl
Quoted 5 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.
Quoted 7 lines
Quoted 3 lines
These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit.
NACK. The benefit is improved standards compliance. One day we could find QEMU being ported to environment which conflicts with these symbols.
We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance.
Quoted 3 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
Quoted 23 lines
Quoted 23 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- HACKING | 4 ---- 1 file changed, 4 deletions(-)HACKING +0
-5@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign --MST
Quoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Blue Swirl
Quoted 8 lines · Michael S. Tsirkin
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit.
Quoted 5 lines
Quoted 3 lines
NACK. The benefit is improved standards compliance. One day we could find QEMU being ported to environment which conflicts with these symbols.
We are talking about stuff like
__kvm_pv_eoi- so the chance is exactly 0.
How do you know for sure? The implementation could be that symbols are converted by the compiler to use underscore prefixes.
Quoted 2 lines
And if it does happen then you run a simple script and fix this one instance.
Then why not fix now? How about kvm_pv_eoi_state?
Quoted 8 lines
Quoted 4 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
There are plenty of bad things wrt style, but the plan is to get rid of those eventually. It takes time since global reformatting etc. to enforce the rules is not accepted.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- HACKING | 4 ---- 1 file changed, 4 deletions(-)HACKING +0
-5@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign --MST
Quoted 3 lines · Michael S. Tsirkin
We are talking about stuff like
__kvm_pv_eoi- so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance.
Why not just use a name that doesn't use a double underscore in the first place? The C standard specifically allows single underscore + lowercase to give things other than the implementation part of the underscore-namespace. In this case, "_kvm_pv_eoi" would be OK.
Quoted 7 lines
Quoted 3 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
HACKING and CODING_STYLE contain a number of rules which the existing codebase doesn't fully conform to. The idea is to incrementally improve consistency and correctness.
-- PMM
Quoted 10 lines · Peter Maydell
Quoted 3 lines · Michael S. Tsirkin
We are talking about stuff like
__kvm_pv_eoi- so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance.Why not just use a name that doesn't use a double underscore in the first place? The C standard specifically allows single underscore + lowercase to give things other than the implementation part of the underscore-namespace. In this case, "
_kvm_pv_eoi" would be OK.
No it wouldn't, _kvm_pv_eoi is a file scope identifier, and names beginning with underscore are reserved in this context.
Quoted 15 lines
Quoted 8 lines
Quoted 4 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
HACKING and
CODING_STYLEcontain a number of rules which the existing codebase doesn't fully conform to. The idea is to incrementally improve consistency and correctness.-- PMM
Signature · 1 line
mailto:[email protected]
Quoted 7 lines · malc
Quoted 5 lines · Peter Maydell
Quoted 3 lines · Michael S. Tsirkin
We are talking about stuff like
__kvm_pv_eoi- so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance.
Quoted 8 lines
Quoted 5 lines
Why not just use a name that doesn't use a double underscore in the first place? The C standard specifically allows single underscore + lowercase to give things other than the implementation part of the underscore-namespace. In this case, "
_kvm_pv_eoi" would be OK.No it wouldn't,
_kvm_pv_eoiis a file scope identifier, and names beginning with underscore are reserved in this context.
Looks like they are and I missed that.
Maybe we should add that to HACKING.
Quoted 18 lines
Quoted 15 lines
Quoted 8 lines
Quoted 4 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
HACKING and
CODING_STYLEcontain a number of rules which the existing codebase doesn't fully conform to. The idea is to incrementally improve consistency and correctness.-- PMM
Signature · 1 line
mailto:[email protected]
Quoted 10 lines · Peter Maydell
Quoted 3 lines · Michael S. Tsirkin
We are talking about stuff like
__kvm_pv_eoi- so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance.Why not just use a name that doesn't use a double underscore in the first place? The C standard specifically allows single underscore + lowercase to give things other than the implementation part of the underscore-namespace. In this case, "
_kvm_pv_eoi" would be OK.
BTW this is exactly what v2 of my patch did but Blue Swirl nacked this too.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available.
Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself.
Quoted 5 lines
HACKING and
CODING_STYLEcontain a number of rules which the existing codebase doesn't fully conform to. The idea is to incrementally improve consistency and correctness.-- PMM
How about fixing HACKING itself? It recommends using ram_addr_t.
Am 28.08.2012 18:01, schrieb Michael S. Tsirkin:
Quoted 8 lines
We copied HACKING from libvirt but it has some bogus stuff:
neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts.These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit.
It was generally not accepted to do Coding Style cleanups in the code base so you will find lots of violations for any rule if you invest the time to search. That's no reason to abolish any such rule.
Cc'ing malc, who has "violently" fought for this particular rule.
I also think that asking people not to introduce new ..._t typedefs is different from asking to use appropriate existing typedefs, so HACKING is not really contradicting itself in that aspect.
Regards,
Andreas
Quoted 22 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- HACKING | 4 ---- 1 file changed, 4 deletions(-)HACKING +0
-4@@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg