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

Use std::variant in ManifestYamlPopulator #4081

Merged
merged 6 commits into from Jan 24, 2024

Conversation

msftrubengu
Copy link
Contributor

@msftrubengu msftrubengu commented Jan 17, 2024

This PR uses std::variant instead of member pointers to manifest structs to be references used by the processing functions. std::variant contains pointers to manifest structs to be populated and is passed to the FieldProcessInfo.ProcessFunc. Each processing function is responsible of calling std::get or std::get_if with the correct pointer type. ValidateAndProcessFields takes the std::variant and pass it to the processing functions.

Processing functions that required the member types now take a pointer to the manifest struct. Some of these can go in the anonymous namespace, but I didn't want to add more moves.

The change will make implementing shadow manifests easier.

This comment has been minimized.

@msftrubengu msftrubengu marked this pull request as ready for review January 18, 2024 03:20
@msftrubengu msftrubengu requested a review from a team as a code owner January 18, 2024 03:20
{ "ProductCode", [this](const YAML::Node& value)->ValidationErrors { m_p_installer->ProductCode = value.as<std::string>(); return {}; } },
{
"InstallerType",
[forRootFields](const YAML::Node& value, std::any& any)->ValidationErrors
Copy link
Member

Choose a reason for hiding this comment

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

This seems fragile, I would really prefer to avoid a capture. But I'm guessing it isn't nearly as easy to do as it is to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried splitting GetRootFieldProcessInfo and calling ValidateAndProcessFields with root, root installer and root localization separately. but that will result in several ManifestError::FieldUnknown warning. I don't want to deal with looking at the errors and remove them.

I removed the capture, but it still needs to any_cast to manifest or installer/localization. I'm not sure if the capture is better than this try catch approach.

},
{
"Documentations",
[this, forRootFields](const YAML::Node& value, std::any& any)->ValidationErrors
Copy link
Member

Choose a reason for hiding this comment

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

I thought the point of this was to get rid of the this capture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't get rid of it in all of them :(

@JohnMcPMS
Copy link
Member

You are going to hate me, but I think std::variant might be the better approach. I don't think it is reliant on typeid, which we want to avoid due to size bloat. It will work the same way, except that you have to up front define the set of pointer types you would accept.

@msftrubengu msftrubengu changed the title Use std::any in ManifestYamlPopulator Use std::variant in ManifestYamlPopulator Jan 24, 2024
@msftrubengu msftrubengu merged commit e2d7202 into microsoft:master Jan 24, 2024
8 checks passed
@msftrubengu msftrubengu deleted the any_populator branch April 19, 2024 22:14
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.

None yet

2 participants