Re: [PATCH v3 01/19] target/arm: Rename KVM set_feature() as kvm_set_feature()
On 4/19/20 9:58 PM, Peter Maydell wrote: On Sun, 19 Apr 2020 at 17:31, Philippe Mathieu-Daudé wrote: On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: On 3/16/20 9:16 PM, Richard Henderson wrote: On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: +++ b/target/arm/kvm32.c @@ -22,7 +22,7 @@ #include "internals.h" #include "qemu/log.h" -static inline void set_feature(uint64_t *features, int feature) +static inline void kvm_set_feature(uint64_t *features, int feature) Why, what's wrong with the existing name? Peter suggested the rename here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html In that message I suggest that if you move the set_feature() function to cpu.h (which is included in lots of places) then that is too generic a name to use for it. The function of the same name here in kvm32.c is fine, because it's 'static inline' and only visible in this file, so the bar for naming is lower. (In fact, it's a demonstration of why you don't want a generic name like 'set_feature' in a widely included header file.) And your suggestion is indeed obviously correct... Apparently after 19 months rebasing this work I'm not seeing clearly. Thanks again!
Re: [PATCH v3 01/19] target/arm: Rename KVM set_feature() as kvm_set_feature()
On Sun, 19 Apr 2020 at 17:31, Philippe Mathieu-Daudé wrote: > > On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: > > On 3/16/20 9:16 PM, Richard Henderson wrote: > >> On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: > >>> +++ b/target/arm/kvm32.c > >>> @@ -22,7 +22,7 @@ > >>> #include "internals.h" > >>> #include "qemu/log.h" > >>> -static inline void set_feature(uint64_t *features, int feature) > >>> +static inline void kvm_set_feature(uint64_t *features, int feature) > >> > >> Why, what's wrong with the existing name? > > Peter suggested the rename here: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html In that message I suggest that if you move the set_feature() function to cpu.h (which is included in lots of places) then that is too generic a name to use for it. The function of the same name here in kvm32.c is fine, because it's 'static inline' and only visible in this file, so the bar for naming is lower. (In fact, it's a demonstration of why you don't want a generic name like 'set_feature' in a widely included header file.) thanks -- PMM
Re: [PATCH v3 01/19] target/arm: Rename KVM set_feature() as kvm_set_feature()
On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: On 3/16/20 9:16 PM, Richard Henderson wrote: On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: +++ b/target/arm/kvm32.c @@ -22,7 +22,7 @@ #include "internals.h" #include "qemu/log.h" -static inline void set_feature(uint64_t *features, int feature) +static inline void kvm_set_feature(uint64_t *features, int feature) Why, what's wrong with the existing name? Peter suggested the rename here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html Plus, with patch 2, you can just remove these. Since they don't have the same prototype, they clash: target/arm/kvm64.c:450:20: error: conflicting types for ‘set_feature’ static inline void set_feature(uint64_t *features, int feature) ^~~ In file included from target/arm/kvm64.c:30:0: target/arm/internals.h:30:20: note: previous definition of ‘set_feature’ was here static inline void set_feature(CPUARMState *env, int feature) ^~~ target/arm/kvm64.c:455:20: error: conflicting types for ‘unset_feature’ static inline void unset_feature(uint64_t *features, int feature) ^ In file included from target/arm/kvm64.c:30:0: target/arm/internals.h:35:20: note: previous definition of ‘unset_feature’ was here static inline void unset_feature(CPUARMState *env, int feature) ^ rules.mak:69: recipe for target 'target/arm/kvm64.o' failed make[1]: *** [target/arm/kvm64.o] Error 1 The prototypes are different: void set_feature(uint64_t *features, int feature) void set_feature(CPUARMState *env, int feature) Anyway you are right, I'll use the later prototype instead, thanks. There are ~180 uses of set_feature(CPUARMState,...) and 10 of set_feature(uint64_t,...) (kvm32:4 kvm64:6). We are going to remove kvm32, so replacing 180 set_feature(env) by set_feature(env->features) seems a waste. If you prefer to avoid renaming as kvm_set_feature() another option is to move the declaration in a local "features.h" header that would not be included by kvm*.c. The main problem is the use of the ARMHostCPUFeatures structure which apparently was introduced similar to a CPUClass (commit a96c0514ab7) then lost this in commit c4487d76d52.
Re: [PATCH v3 01/19] target/arm: Rename KVM set_feature() as kvm_set_feature()
On 3/16/20 9:16 PM, Richard Henderson wrote: On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: +++ b/target/arm/kvm32.c @@ -22,7 +22,7 @@ #include "internals.h" #include "qemu/log.h" -static inline void set_feature(uint64_t *features, int feature) +static inline void kvm_set_feature(uint64_t *features, int feature) Why, what's wrong with the existing name? Plus, with patch 2, you can just remove these. The prototypes are different: void set_feature(uint64_t *features, int feature) void set_feature(CPUARMState *env, int feature) Anyway you are right, I'll use the later prototype instead, thanks.
Re: [PATCH v3 01/19] target/arm: Rename KVM set_feature() as kvm_set_feature()
On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: > +++ b/target/arm/kvm32.c > @@ -22,7 +22,7 @@ > #include "internals.h" > #include "qemu/log.h" > > -static inline void set_feature(uint64_t *features, int feature) > +static inline void kvm_set_feature(uint64_t *features, int feature) Why, what's wrong with the existing name? Plus, with patch 2, you can just remove these. r~
