Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
>>> On 16.02.17 at 12:59, wrote: > On 16/02/17 11:10, Jan Beulich wrote: > On 16.02.17 at 12:01, wrote: >>> On 16/02/17 10:48, Jan Beulich wrote: >>> On 16.02.17 at 11:40, wrote: > On 16/02/17 10:27, Jan Beulich wrote: > On 15.02.17 at 19:10, wrote: >>> generated/autoconf.h is already included automatically so CONFIG_* >>> defines >>> are >>> avaialble. However, the companion macros such as IS_ENABLED() are not >>> included. >>> >>> Include them uniformally everywhere. >> Well, if you really think this is a good idea, I'm not going to stand in >> the way, but why do we need this included everywhere? Many files >> don't even care about any CONFIG_*, and hence even less so about >> kconfig.h. > I am sorry, but you are complaining if I include it unilaterally, and > also complaining if I include kconfig.h in the specific location where I > need it. These are mutually exclusive. I don't understand - when did I complain about its inclusion where it's needed? Iirc my complaint was about you adding the inclusion to */config.h without that header actually using the macros. My point really is that ideally each C file would get as little cruft as possible, while at present quite a number of header are being included by virtually every source file. >>> Your complaint was specifically about me adding it to paging.h so I >>> could use IS_ENABLED() and not out-of-line a trivial function. >> Oh, that one: There my view was the other way around: No need >> to include yet another header in one which already gets included >> everywhere, when the new function could easily be out of line (as >> not being performance critical). >> >>> As for general availably, while I agree in general that we have far too >>> much stuff included by default (I have some plans for that), the >>> contents of kconfig.h is fairly small, and exactly the same category of >>> information as config.h >>> >>> I am looking to push for the use of IS_ENABLED() in preference to #ifdef >>> where possible, to reduce code-rot. >> Which makes sense, but won't affect said source files not using any >> CONFIG_* in the first place. > > We already include CONFIG_* everywhere. All this change does is > consistently add IS_ENABLED() alongside, so it can be used when CONFIG_* > are available. The relevant aspect isn't CONFIG_* being available, but any of them being actually used. > If we have occasion in the future to reconsider having the CONFIG_* > variables unilaterally included, then fine, but the current state of the > code is the worst of all options. I don't think so, but as said, I'm not meaning to stand in the way of this patch going in (as making the current situation only marginally worse). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
Hi Jan, On 16/02/17 11:10, Jan Beulich wrote: On 16.02.17 at 12:01, wrote: On 16/02/17 10:48, Jan Beulich wrote: On 16.02.17 at 11:40, wrote: On 16/02/17 10:27, Jan Beulich wrote: On 15.02.17 at 19:10, wrote: generated/autoconf.h is already included automatically so CONFIG_* defines are avaialble. However, the companion macros such as IS_ENABLED() are not included. Include them uniformally everywhere. Well, if you really think this is a good idea, I'm not going to stand in the way, but why do we need this included everywhere? Many files don't even care about any CONFIG_*, and hence even less so about kconfig.h. I am sorry, but you are complaining if I include it unilaterally, and also complaining if I include kconfig.h in the specific location where I need it. These are mutually exclusive. I don't understand - when did I complain about its inclusion where it's needed? Iirc my complaint was about you adding the inclusion to */config.h without that header actually using the macros. My point really is that ideally each C file would get as little cruft as possible, while at present quite a number of header are being included by virtually every source file. Your complaint was specifically about me adding it to paging.h so I could use IS_ENABLED() and not out-of-line a trivial function. Oh, that one: There my view was the other way around: No need to include yet another header in one which already gets included everywhere, when the new function could easily be out of line (as not being performance critical). As for general availably, while I agree in general that we have far too much stuff included by default (I have some plans for that), the contents of kconfig.h is fairly small, and exactly the same category of information as config.h I am looking to push for the use of IS_ENABLED() in preference to #ifdef where possible, to reduce code-rot. Which makes sense, but won't affect said source files not using any CONFIG_* in the first place. At least on ARM, we need CONFIG_* everywhere as the definitions of types and structure will change whether you are compiling for ARM64 or ARM. I would expect the same for common code. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
On 16/02/17 11:10, Jan Beulich wrote: On 16.02.17 at 12:01, wrote: >> On 16/02/17 10:48, Jan Beulich wrote: >> On 16.02.17 at 11:40, wrote: On 16/02/17 10:27, Jan Beulich wrote: On 15.02.17 at 19:10, wrote: >> generated/autoconf.h is already included automatically so CONFIG_* >> defines >> are >> avaialble. However, the companion macros such as IS_ENABLED() are not >> included. >> >> Include them uniformally everywhere. > Well, if you really think this is a good idea, I'm not going to stand in > the way, but why do we need this included everywhere? Many files > don't even care about any CONFIG_*, and hence even less so about > kconfig.h. I am sorry, but you are complaining if I include it unilaterally, and also complaining if I include kconfig.h in the specific location where I need it. These are mutually exclusive. >>> I don't understand - when did I complain about its inclusion where >>> it's needed? Iirc my complaint was about you adding the inclusion >>> to */config.h without that header actually using the macros. My >>> point really is that ideally each C file would get as little cruft as >>> possible, while at present quite a number of header are being >>> included by virtually every source file. >> Your complaint was specifically about me adding it to paging.h so I >> could use IS_ENABLED() and not out-of-line a trivial function. > Oh, that one: There my view was the other way around: No need > to include yet another header in one which already gets included > everywhere, when the new function could easily be out of line (as > not being performance critical). > >> As for general availably, while I agree in general that we have far too >> much stuff included by default (I have some plans for that), the >> contents of kconfig.h is fairly small, and exactly the same category of >> information as config.h >> >> I am looking to push for the use of IS_ENABLED() in preference to #ifdef >> where possible, to reduce code-rot. > Which makes sense, but won't affect said source files not using any > CONFIG_* in the first place. We already include CONFIG_* everywhere. All this change does is consistently add IS_ENABLED() alongside, so it can be used when CONFIG_* are available. If we have occasion in the future to reconsider having the CONFIG_* variables unilaterally included, then fine, but the current state of the code is the worst of all options. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
>>> On 16.02.17 at 12:01, wrote: > On 16/02/17 10:48, Jan Beulich wrote: > On 16.02.17 at 11:40, wrote: >>> On 16/02/17 10:27, Jan Beulich wrote: >>> On 15.02.17 at 19:10, wrote: > generated/autoconf.h is already included automatically so CONFIG_* > defines > are > avaialble. However, the companion macros such as IS_ENABLED() are not > included. > > Include them uniformally everywhere. Well, if you really think this is a good idea, I'm not going to stand in the way, but why do we need this included everywhere? Many files don't even care about any CONFIG_*, and hence even less so about kconfig.h. >>> I am sorry, but you are complaining if I include it unilaterally, and >>> also complaining if I include kconfig.h in the specific location where I >>> need it. These are mutually exclusive. >> I don't understand - when did I complain about its inclusion where >> it's needed? Iirc my complaint was about you adding the inclusion >> to */config.h without that header actually using the macros. My >> point really is that ideally each C file would get as little cruft as >> possible, while at present quite a number of header are being >> included by virtually every source file. > > Your complaint was specifically about me adding it to paging.h so I > could use IS_ENABLED() and not out-of-line a trivial function. Oh, that one: There my view was the other way around: No need to include yet another header in one which already gets included everywhere, when the new function could easily be out of line (as not being performance critical). > As for general availably, while I agree in general that we have far too > much stuff included by default (I have some plans for that), the > contents of kconfig.h is fairly small, and exactly the same category of > information as config.h > > I am looking to push for the use of IS_ENABLED() in preference to #ifdef > where possible, to reduce code-rot. Which makes sense, but won't affect said source files not using any CONFIG_* in the first place. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
On 16/02/17 10:48, Jan Beulich wrote: On 16.02.17 at 11:40, wrote: >> On 16/02/17 10:27, Jan Beulich wrote: >> On 15.02.17 at 19:10, wrote: generated/autoconf.h is already included automatically so CONFIG_* defines are avaialble. However, the companion macros such as IS_ENABLED() are not included. Include them uniformally everywhere. >>> Well, if you really think this is a good idea, I'm not going to stand in >>> the way, but why do we need this included everywhere? Many files >>> don't even care about any CONFIG_*, and hence even less so about >>> kconfig.h. >> I am sorry, but you are complaining if I include it unilaterally, and >> also complaining if I include kconfig.h in the specific location where I >> need it. These are mutually exclusive. > I don't understand - when did I complain about its inclusion where > it's needed? Iirc my complaint was about you adding the inclusion > to */config.h without that header actually using the macros. My > point really is that ideally each C file would get as little cruft as > possible, while at present quite a number of header are being > included by virtually every source file. Your complaint was specifically about me adding it to paging.h so I could use IS_ENABLED() and not out-of-line a trivial function. As for general availably, while I agree in general that we have far too much stuff included by default (I have some plans for that), the contents of kconfig.h is fairly small, and exactly the same category of information as config.h I am looking to push for the use of IS_ENABLED() in preference to #ifdef where possible, to reduce code-rot. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
>>> On 16.02.17 at 11:40, wrote: > On 16/02/17 10:27, Jan Beulich wrote: > On 15.02.17 at 19:10, wrote: >>> generated/autoconf.h is already included automatically so CONFIG_* defines >>> are >>> avaialble. However, the companion macros such as IS_ENABLED() are not >>> included. >>> >>> Include them uniformally everywhere. >> Well, if you really think this is a good idea, I'm not going to stand in >> the way, but why do we need this included everywhere? Many files >> don't even care about any CONFIG_*, and hence even less so about >> kconfig.h. > > I am sorry, but you are complaining if I include it unilaterally, and > also complaining if I include kconfig.h in the specific location where I > need it. These are mutually exclusive. I don't understand - when did I complain about its inclusion where it's needed? Iirc my complaint was about you adding the inclusion to */config.h without that header actually using the macros. My point really is that ideally each C file would get as little cruft as possible, while at present quite a number of header are being included by virtually every source file. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
On 16/02/17 10:27, Jan Beulich wrote: On 15.02.17 at 19:10, wrote: >> generated/autoconf.h is already included automatically so CONFIG_* defines >> are >> avaialble. However, the companion macros such as IS_ENABLED() are not >> included. >> >> Include them uniformally everywhere. > Well, if you really think this is a good idea, I'm not going to stand in > the way, but why do we need this included everywhere? Many files > don't even care about any CONFIG_*, and hence even less so about > kconfig.h. I am sorry, but you are complaining if I include it unilaterally, and also complaining if I include kconfig.h in the specific location where I need it. These are mutually exclusive. I do prefer this approach, overall. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
>>> On 15.02.17 at 19:10, wrote: > generated/autoconf.h is already included automatically so CONFIG_* defines are > avaialble. However, the companion macros such as IS_ENABLED() are not > included. > > Include them uniformally everywhere. Well, if you really think this is a good idea, I'm not going to stand in the way, but why do we need this included everywhere? Many files don't even care about any CONFIG_*, and hence even less so about kconfig.h. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
Hi Andrew, On 15/02/2017 18:10, Andrew Cooper wrote: generated/autoconf.h is already included automatically so CONFIG_* defines are avaialble. However, the companion macros such as IS_ENABLED() are not NIT: s/avaialble/available/ included. Include them uniformally everywhere. Signed-off-by: Andrew Cooper Acked-by: Julien Grall Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 3/3] xen/include: Include xen/kconfig.h automatically
generated/autoconf.h is already included automatically so CONFIG_* defines are avaialble. However, the companion macros such as IS_ENABLED() are not included. Include them uniformally everywhere. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Stefano Stabellini CC: Julien Grall --- xen/drivers/acpi/osl.c| 1 - xen/include/asm-arm/alternative.h | 1 - xen/include/xen/config.h | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index f75dfb7..94dbf04 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -37,7 +37,6 @@ #include #include #include -#include #define _COMPONENT ACPI_OS_SERVICES ACPI_MODULE_NAME("osl") diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h index c9740b8..6cc9d0d 100644 --- a/xen/include/asm-arm/alternative.h +++ b/xen/include/asm-arm/alternative.h @@ -2,7 +2,6 @@ #define __ASM_ALTERNATIVE_H #include -#include #ifdef CONFIG_HAS_ALTERNATIVE diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h index 473c5e8..9f39687 100644 --- a/xen/include/xen/config.h +++ b/xen/include/xen/config.h @@ -7,7 +7,7 @@ #ifndef __XEN_CONFIG_H__ #define __XEN_CONFIG_H__ -#include +#include #ifndef __ASSEMBLY__ #include -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel