Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UR] Allow for extending enums #626

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

veselypeta
Copy link
Contributor

@veselypeta veselypeta commented Jun 16, 2023

Method to extend existing enums:

  • Introduces a new yaml syntax - enum can now include and extend field to indicate that this extends an existing enum
  • When parsing this new type, it inserts all the etors into already existing etor
  • We have to manually refresh the meta data structure - to ensure downstream changes as picked up such as validation/loader etc.

Closes #606

scripts/parse_specs.py Outdated Show resolved Hide resolved
@kbenzie
Copy link
Contributor

kbenzie commented Jun 16, 2023

Once the design is done it should be documented in scripts/core/CONTRIB.rst and the YaML.md.

@veselypeta veselypeta force-pushed the petr/606/extend-enums branch 2 times, most recently from d326466 to 125b996 Compare June 19, 2023 13:41
@veselypeta veselypeta marked this pull request as ready for review June 19, 2023 13:43
@veselypeta veselypeta force-pushed the petr/606/extend-enums branch 2 times, most recently from 49eeef5 to a5a4a18 Compare June 19, 2023 13:48
Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - minor comments

scripts/core/CONTRIB.rst Outdated Show resolved Hide resolved
scripts/core/CONTRIB.rst Show resolved Hide resolved
scripts/core/CONTRIB.rst Show resolved Hide resolved
scripts/core/CONTRIB.rst Outdated Show resolved Hide resolved
scripts/parse_specs.py Outdated Show resolved Hide resolved
@veselypeta
Copy link
Contributor Author

Another idea I had. Instead of requiring every extended enum to provide a unique value - we could just specify a starting range.
i.e.

type: 'enum'
name: $x_device_info_t
desc: "a desc"
extended: true
offset: "0x1000"
etors:
  - name: "ETOR_1"
    desc: "desc"
  - name: "ETOR_2"
    desc: "desc 2"

So enums will automatically be allocated sequentially

@pbalcer
Copy link
Contributor

pbalcer commented Jun 21, 2023

Another idea I had. Instead of requiring every extended enum to provide a unique value - we could just specify a starting range. i.e.

type: 'enum'
name: $x_device_info_t
desc: "a desc"
extended: true
offset: "0x1000"
etors:
  - name: "ETOR_1"
    desc: "desc"
  - name: "ETOR_2"
    desc: "desc 2"

So enums will automatically be allocated sequentially

Do the specific values matter? If not, would it be possible to, for example, automatically allocate ranges for new extensions from the top of the enum's range? e.g.,

enum FOO {
   FOO_OPTION_A = 1,
   FOO_OPTION_B = 2,
    ....

   EXT2_OPTION_A,
   EXT2_OPTION_B,
   EXT1_OPTION_A,
   EXT1_OPTION_B,
   MAX = UINT32_MAX,
};

@veselypeta
Copy link
Contributor Author

Do the specific values matter? If not, would it be possible to, for example, automatically allocate ranges for new extensions from the top of the enum's range? e.g.,

I don't think so, but I think that assigning values could be problematic. Possibly adding a new experimental feature might change the order in which they are allocated and modify the range.

@veselypeta
Copy link
Contributor Author

I've decided not to introduce offset or automatically allocating experimental feature ranges yet, instead enforcing that each etor must have a unique value. I feel like we can easily iterate on this and add this at a later point.

@veselypeta veselypeta requested a review from pbalcer June 21, 2023 11:31
@veselypeta veselypeta merged commit 24f5e01 into oneapi-src:main Jun 21, 2023
18 checks passed
@veselypeta veselypeta deleted the petr/606/extend-enums branch June 21, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for extending existing enumerations in experimental feature yaml files
3 participants