| Message ID | 20180321115211.17937-16-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: add #if pre-processor conditions to generated code | expand |
Subject: explicitly 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/common.py | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index 60c1d0a783..68a567f53f 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -2032,14 +2032,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; What excactly in your series depends on this? What safeguards do you propose to ensure an enumeration with conditionals is compiled only with the exact same conditionals within the same program? Example of the kind of deathtrap to guard against: compile 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. Yes, I know a similar deathtrap will be set up for struct and union types. No excuse for ignoring either of the two.
Hi On Fri, Jun 22, 2018 at 10:08 AM, Markus Armbruster <armbru@redhat.com> wrote: > Subject: explicitly > > 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/common.py | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index 60c1d0a783..68a567f53f 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -2032,14 +2032,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; > > What excactly in your series depends on this? > > What safeguards do you propose to ensure an enumeration with > conditionals is compiled only with the exact same conditionals within > the same program? > > Example of the kind of deathtrap to guard against: compile > > 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. This was a trick used in previous iterations. Now that we have a separate target schema and generated code/headers, and since we have poisoned identifiers, this should never happen. > Yes, I know a similar deathtrap will be set up for struct and union > types. No excuse for ignoring either of the two. Having gaps in the enums makes it harder to iterate over all values, and we use more memory than necessary when allocating based on MAX value. It's not a big problem, but I consider this more important than this artificially made up broken build example.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Fri, Jun 22, 2018 at 10:08 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Subject: explicitly >> >> 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/common.py | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index 60c1d0a783..68a567f53f 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -2032,14 +2032,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; >> >> What excactly in your series depends on this? >> >> What safeguards do you propose to ensure an enumeration with >> conditionals is compiled only with the exact same conditionals within >> the same program? >> >> Example of the kind of deathtrap to guard against: compile >> >> 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. > > This was a trick used in previous iterations. Now that we have a > separate target schema and generated code/headers, and since we have > poisoned identifiers, this should never happen. > >> Yes, I know a similar deathtrap will be set up for struct and union >> types. No excuse for ignoring either of the two. > > Having gaps in the enums makes it harder to iterate over all values, > and we use more memory than necessary when allocating based on MAX > value. > > It's not a big problem, but I consider this more important than this > artificially made up broken build example. Made up yes, but the making up comes from painful experience debugging real programs :) I'm not at all opposed to "contracting" enums. I just want to be persuaded it's safe. I think your argument is "Now that we have a separate target schema and generated code/headers, and since we have poisoned identifiers, this should never happen." Can you elaborate? If that goes as I expect it to go, the next request will be "now put that into your commit message" :)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 60c1d0a783..68a567f53f 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -2032,14 +2032,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/common.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)