Skip to content
New issue

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

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

Already on GitHub? # to your account

Factor out functions for auto-discovery parsing #108

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

pcolberg
Copy link
Contributor

@pcolberg pcolberg commented Apr 22, 2022

This factors out parsing of auto-discovery string sections into separate functions to improve the readability and maintenance of acl_load_device_def_from_str(). Care was taken not to modify the functional behaviour, by carrying out the refactoring twice and ensuring that the results are identical.

A choice had to be made between factoring out parsing of individual section entries, e.g., a single kernel argument, versus factoring out entire loops, e.g., all kernel arguments. The latter was chosen since the data structures vary (vector versus unordered_map), and, importantly, the number of fields vary between variable and fixed counts. For variable count, the number of fields is read for each entry; whereas for fixed count, the number of fields is read once for all entries. By factoring out the entire loop, the field count parsing is contained within the function in both cases.

@pcolberg pcolberg self-assigned this Apr 22, 2022
@pcolberg pcolberg marked this pull request as draft April 22, 2022 02:20
src/acl_auto_configure.cpp Outdated Show resolved Hide resolved
src/acl_auto_configure.cpp Outdated Show resolved Hide resolved
@pcolberg pcolberg changed the title Factor out function for auto-discovery parsing of kernel arguments Factor out functions for auto-discovery parsing Apr 22, 2022
@pcolberg pcolberg added the enhancement New feature or request label Apr 22, 2022
@pcolberg pcolberg added this to the 2022.3 milestone Apr 22, 2022
@pcolberg pcolberg force-pushed the auto-discovery branch 4 times, most recently from cd4bec6 to c96f2d7 Compare April 23, 2022 00:37
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
This amends intel#109

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
@pcolberg pcolberg marked this pull request as ready for review April 25, 2022 22:42
Copy link
Contributor

@sherry-yuan sherry-yuan left a comment

Choose a reason for hiding this comment

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

Thanks Peter, the restructuring looks good to me.

(merge after tests passes tho)

Copy link
Contributor

@zibaiwan zibaiwan left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks Peter!

@pcolberg pcolberg merged commit e1ff665 into intel:main Apr 29, 2022
@pcolberg pcolberg deleted the auto-discovery branch April 29, 2022 22:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants