| Message ID | 20170911110623.24981-18-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hi, | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > The C standard has the initial value at 0 and the subsequent values > incremented by 1. No need to set this explicitely. > > This will prevent from artificial "gaps" when compiling out some enum > values and having unnecessarily large MAX values & enums arrays. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi.py | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 94b735d8d6..074ee221a1 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { > ''', > c_name=c_name(name)) > > - i = 0 > for value in enum_values: > ret += mcgen(''' > - %(c_enum)s = %(i)d, > + %(c_enum)s, > ''', > - c_enum=c_enum_const(name, value, prefix), > - i=i) > - i += 1 > + c_enum=c_enum_const(name, value, prefix)) > > ret += mcgen(''' > } %(c_name)s; Recapitulate review of v2: this risks entertaining mishaps like compiling this one typedef enum Color { COLOR_WHITE, #if defined(NEED_CPU_H) #if defined(TARGET_S390X) COLOR_BLUE, #endif /* defined(TARGET_S390X) */ #endif /* defined(NEED_CPU_H) */ COLOR_BLACK, } Color; in s390x-code (COLOR_BLACK = 2) and in target-independent code (COLOR_BLACK = 1), then linking the two together. Same issue for struct members and such (previous patch). What's our story on preventing disaster here? In the long run, we want to split the generated code so that target-specific and target-independent code are separate, and each part is always compiled with consistent preprocessor symbols. But I'm afraid that's not in the card right now. I therefore proposed the stupidest temporary stopgap that could possibly work: apply conditionals *only* to qmp-introspect.c, leave everything unconditional elsewhere.
On Thu, Dec 7, 2017 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> The C standard has the initial value at 0 and the subsequent values >> incremented by 1. No need to set this explicitely. >> >> This will prevent from artificial "gaps" when compiling out some enum >> values and having unnecessarily large MAX values & enums arrays. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> scripts/qapi.py | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index 94b735d8d6..074ee221a1 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { >> ''', >> c_name=c_name(name)) >> >> - i = 0 >> for value in enum_values: >> ret += mcgen(''' >> - %(c_enum)s = %(i)d, >> + %(c_enum)s, >> ''', >> - c_enum=c_enum_const(name, value, prefix), >> - i=i) >> - i += 1 >> + c_enum=c_enum_const(name, value, prefix)) >> >> ret += mcgen(''' >> } %(c_name)s; > > Recapitulate review of v2: this risks entertaining mishaps like > compiling this one > > typedef enum Color { > COLOR_WHITE, > #if defined(NEED_CPU_H) > #if defined(TARGET_S390X) > COLOR_BLUE, > #endif /* defined(TARGET_S390X) */ > #endif /* defined(NEED_CPU_H) */ > COLOR_BLACK, > } Color; > > in s390x-code (COLOR_BLACK = 2) and in target-independent code > (COLOR_BLACK = 1), then linking the two together. > > Same issue for struct members and such (previous patch). > > What's our story on preventing disaster here? > > In the long run, we want to split the generated code so that > target-specific and target-independent code are separate, and each part > is always compiled with consistent preprocessor symbols. But I'm afraid > that's not in the card right now. Eh, I need to refresh my memories about that series, but I think that's what I did in v3 It doesn't use the NEED_CPU_H trick. It has a seperate per-target target.json > > I therefore proposed the stupidest temporary stopgap that could possibly > work: apply conditionals *only* to qmp-introspect.c, leave everything > unconditional elsewhere. I don't like that idea much and I don't think we need that restriction, but I need to get back to that series on some point (probably after you finish the review).
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > On Thu, Dec 7, 2017 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >> >>> The C standard has the initial value at 0 and the subsequent values >>> incremented by 1. No need to set this explicitely. >>> >>> This will prevent from artificial "gaps" when compiling out some enum >>> values and having unnecessarily large MAX values & enums arrays. >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> scripts/qapi.py | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>> index 94b735d8d6..074ee221a1 100644 >>> --- a/scripts/qapi.py >>> +++ b/scripts/qapi.py >>> @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { >>> ''', >>> c_name=c_name(name)) >>> >>> - i = 0 >>> for value in enum_values: >>> ret += mcgen(''' >>> - %(c_enum)s = %(i)d, >>> + %(c_enum)s, >>> ''', >>> - c_enum=c_enum_const(name, value, prefix), >>> - i=i) >>> - i += 1 >>> + c_enum=c_enum_const(name, value, prefix)) >>> >>> ret += mcgen(''' >>> } %(c_name)s; >> >> Recapitulate review of v2: this risks entertaining mishaps like >> compiling this one >> >> typedef enum Color { >> COLOR_WHITE, >> #if defined(NEED_CPU_H) >> #if defined(TARGET_S390X) >> COLOR_BLUE, >> #endif /* defined(TARGET_S390X) */ >> #endif /* defined(NEED_CPU_H) */ >> COLOR_BLACK, >> } Color; >> >> in s390x-code (COLOR_BLACK = 2) and in target-independent code >> (COLOR_BLACK = 1), then linking the two together. >> >> Same issue for struct members and such (previous patch). >> >> What's our story on preventing disaster here? >> >> In the long run, we want to split the generated code so that >> target-specific and target-independent code are separate, and each part >> is always compiled with consistent preprocessor symbols. But I'm afraid >> that's not in the card right now. > > Eh, I need to refresh my memories about that series, but I think > that's what I did in v3 > > It doesn't use the NEED_CPU_H trick. It has a seperate per-target target.json Looking... aha! target.json appears in PATCH 44 (which I haven't even glanced at, yet). The problem appears in PATCH 16, though. Perhaps a bit of patch reshuffling would do. >> I therefore proposed the stupidest temporary stopgap that could possibly >> work: apply conditionals *only* to qmp-introspect.c, leave everything >> unconditional elsewhere. > > I don't like that idea much and I don't think we need that > restriction, but I need to get back to that series on some point > (probably after you finish the review). It's a beefy series, and it's probably best to let me review the largest prefix I can before we dive into discussion.
Hi On Fri, Dec 8, 2017 at 8:50 AM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > >> On Thu, Dec 7, 2017 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >>> >>>> The C standard has the initial value at 0 and the subsequent values >>>> incremented by 1. No need to set this explicitely. >>>> >>>> This will prevent from artificial "gaps" when compiling out some enum >>>> values and having unnecessarily large MAX values & enums arrays. >>>> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> --- >>>> scripts/qapi.py | 7 ++----- >>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>>> index 94b735d8d6..074ee221a1 100644 >>>> --- a/scripts/qapi.py >>>> +++ b/scripts/qapi.py >>>> @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { >>>> ''', >>>> c_name=c_name(name)) >>>> >>>> - i = 0 >>>> for value in enum_values: >>>> ret += mcgen(''' >>>> - %(c_enum)s = %(i)d, >>>> + %(c_enum)s, >>>> ''', >>>> - c_enum=c_enum_const(name, value, prefix), >>>> - i=i) >>>> - i += 1 >>>> + c_enum=c_enum_const(name, value, prefix)) >>>> >>>> ret += mcgen(''' >>>> } %(c_name)s; >>> >>> Recapitulate review of v2: this risks entertaining mishaps like >>> compiling this one >>> >>> typedef enum Color { >>> COLOR_WHITE, >>> #if defined(NEED_CPU_H) >>> #if defined(TARGET_S390X) >>> COLOR_BLUE, >>> #endif /* defined(TARGET_S390X) */ >>> #endif /* defined(NEED_CPU_H) */ >>> COLOR_BLACK, >>> } Color; >>> >>> in s390x-code (COLOR_BLACK = 2) and in target-independent code >>> (COLOR_BLACK = 1), then linking the two together. >>> >>> Same issue for struct members and such (previous patch). >>> >>> What's our story on preventing disaster here? >>> >>> In the long run, we want to split the generated code so that >>> target-specific and target-independent code are separate, and each part >>> is always compiled with consistent preprocessor symbols. But I'm afraid >>> that's not in the card right now. >> >> Eh, I need to refresh my memories about that series, but I think >> that's what I did in v3 >> >> It doesn't use the NEED_CPU_H trick. It has a seperate per-target target.json > > Looking... aha! target.json appears in PATCH 44 (which I haven't even > glanced at, yet). The problem appears in PATCH 16, though. Perhaps a > bit of patch reshuffling would do. What problem appears in patch 16? Some code could be introduced using NEED_CPU_H and link arch & independent code together? It is still true after patch 44. If necessary, I can work on a split-qapi series before the conditionals are added. But the real benefit is only apparent after the conditional are introduced, so I am not motivated to reorder. > >>> I therefore proposed the stupidest temporary stopgap that could possibly >>> work: apply conditionals *only* to qmp-introspect.c, leave everything >>> unconditional elsewhere. >> >> I don't like that idea much and I don't think we need that >> restriction, but I need to get back to that series on some point >> (probably after you finish the review). > > It's a beefy series, and it's probably best to let me review the largest > prefix I can before we dive into discussion. thanks
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Fri, Dec 8, 2017 at 8:50 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >>> On Thu, Dec 7, 2017 at 5:23 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >>>> >>>>> The C standard has the initial value at 0 and the subsequent values >>>>> incremented by 1. No need to set this explicitely. >>>>> >>>>> This will prevent from artificial "gaps" when compiling out some enum >>>>> values and having unnecessarily large MAX values & enums arrays. >>>>> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>> --- >>>>> scripts/qapi.py | 7 ++----- >>>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>>>> index 94b735d8d6..074ee221a1 100644 >>>>> --- a/scripts/qapi.py >>>>> +++ b/scripts/qapi.py >>>>> @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { >>>>> ''', >>>>> c_name=c_name(name)) >>>>> >>>>> - i = 0 >>>>> for value in enum_values: >>>>> ret += mcgen(''' >>>>> - %(c_enum)s = %(i)d, >>>>> + %(c_enum)s, >>>>> ''', >>>>> - c_enum=c_enum_const(name, value, prefix), >>>>> - i=i) >>>>> - i += 1 >>>>> + c_enum=c_enum_const(name, value, prefix)) >>>>> >>>>> ret += mcgen(''' >>>>> } %(c_name)s; >>>> >>>> Recapitulate review of v2: this risks entertaining mishaps like >>>> compiling this one >>>> >>>> typedef enum Color { >>>> COLOR_WHITE, >>>> #if defined(NEED_CPU_H) >>>> #if defined(TARGET_S390X) >>>> COLOR_BLUE, >>>> #endif /* defined(TARGET_S390X) */ >>>> #endif /* defined(NEED_CPU_H) */ >>>> COLOR_BLACK, >>>> } Color; >>>> >>>> in s390x-code (COLOR_BLACK = 2) and in target-independent code >>>> (COLOR_BLACK = 1), then linking the two together. >>>> >>>> Same issue for struct members and such (previous patch). >>>> >>>> What's our story on preventing disaster here? >>>> >>>> In the long run, we want to split the generated code so that >>>> target-specific and target-independent code are separate, and each part >>>> is always compiled with consistent preprocessor symbols. But I'm afraid >>>> that's not in the card right now. >>> >>> Eh, I need to refresh my memories about that series, but I think >>> that's what I did in v3 >>> >>> It doesn't use the NEED_CPU_H trick. It has a seperate per-target target.json >> >> Looking... aha! target.json appears in PATCH 44 (which I haven't even >> glanced at, yet). The problem appears in PATCH 16, though. Perhaps a >> bit of patch reshuffling would do. > > What problem appears in patch 16? Some code could be introduced using > NEED_CPU_H and link arch & independent code together? It's been a while... Generated headers using conditionals must include the headers providing the symbols used in conditionals. Not doing so is an open death trap. PATCH 16 sets up the first instance of the death trap. Or maybe it's PATCH 13. However, including these headers only becomes possible *after* you split off the target-specific stuff in PATCH 44. Do I make any sense? > It is still true > after patch 44. If necessary, I can work on a split-qapi series before > the conditionals are added. But the real benefit is only apparent > after the conditional are introduced, so I am not motivated to > reorder. Understand. As a maintainer, I can ask for improvements, but the only lever I have is saying no. Which should be reserved for cases that are actually wrong, or create inacceptable technical debt. Temporary death traps don't count as either. For cases I merely hate, when asking doesn't help, all I can do is do the work myself. So I did: [PATCH RFC 00/21] Modularize generated QAPI code Message-Id: <20180202130336.24719-1-armbru@redhat.com> [...]
diff --git a/scripts/qapi.py b/scripts/qapi.py index 94b735d8d6..074ee221a1 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1985,14 +1985,11 @@ typedef enum %(c_name)s { ''', c_name=c_name(name)) - i = 0 for value in enum_values: ret += mcgen(''' - %(c_enum)s = %(i)d, + %(c_enum)s, ''', - c_enum=c_enum_const(name, value, prefix), - i=i) - i += 1 + c_enum=c_enum_const(name, value, prefix)) ret += mcgen(''' } %(c_name)s;
The C standard has the initial value at 0 and the subsequent values incremented by 1. No need to set this explicitely. This will prevent from artificial "gaps" when compiling out some enum values and having unnecessarily large MAX values & enums arrays. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- scripts/qapi.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)