Skip to content

Utility functions to transform enum to string and back #148

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Nov 23, 2020
Merged

Utility functions to transform enum to string and back #148

merged 12 commits into from
Nov 23, 2020

Conversation

1e-to
Copy link
Contributor

@1e-to 1e-to commented Oct 20, 2020 •
edited by oleksandr-pavlyk
Loading

This PR closes #102

@1e-to 1e-to requested review from PokhodenkoSA and diptorupd and removed request for PokhodenkoSA October 21, 2020 15:20
PokhodenkoSA
PokhodenkoSA reviewed Nov 12, 2020
View reviewed changes
backends/details/include/dppl_utils_details.h Outdated
Comment on lines 30 to 31
std::string ConvertEnumToStr(info::device_type devTy);
info::device_type ConvertStrToEnum(std::string devTyStr);
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Names of functions is too common. Should be like device_type2string or StrToDeviceType or DeviceTypeFromString.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about DDPL_StrToDeviceType and DPPL_DeviceTypeFromStr? I want to keep the naming convention consistent.

PokhodenkoSA
PokhodenkoSA reviewed Nov 12, 2020
View reviewed changes
backends/details/source/dppl_utils_details.cpp Outdated
#include <sstream>
using namespace cl::sycl;

std::string ConvertEnumToStr(info::device_type devTy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add docstring.

@PokhodenkoSA
Copy link
Contributor

Is there a code which uses conversion from string to enum? I do not see is removal in changes.

@1e-to
Copy link
Contributor Author

1e-to commented Nov 12, 2020

Is there a code which uses conversion from string to enum? I do not see is removal in changes.

@1e-to 1e-to closed this Nov 12, 2020
@1e-to 1e-to reopened this Nov 12, 2020
@1e-to
Copy link
Contributor Author

1e-to commented Nov 12, 2020

Is there a code which uses conversion from string to enum? I do not see is removal in changes.

Now it is not uses in project, but Dipto wrote that it is good design to create pairs of functions for conversion.

PokhodenkoSA
PokhodenkoSA approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA left a comment

Choose a reason for hiding this comment

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

LGTM.

PokhodenkoSA
PokhodenkoSA suggested changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA left a comment

Choose a reason for hiding this comment

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

Small modifications required.

backends/details/include/dppl_utils_details.h Outdated
Comment on lines 30 to 31
std::string StrToDeviceType(info::device_type devTy);
info::device_type DeviceTypeToStr(std::string devTyStr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use DPPL_ prefix or wrap it in namespace dppl. In C we have global namespace for all functions. But this is C++ and internal functions so we could use namspaces.

@PokhodenkoSA PokhodenkoSA merged commit 4fc2005 into IntelPython:master Nov 23, 2020
@1e-to 1e-to deleted the trans_enum_to_str branch March 2, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PokhodenkoSA PokhodenkoSA PokhodenkoSA requested changes

@diptorupd diptorupd Awaiting requested review from diptorupd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Utility functions to transform enum to string and back.
3 participants
@1e-to @PokhodenkoSA @diptorupd