Skip to content

More constexpr schema #227

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

Merged
merged 3 commits into from
Apr 23, 2022
Merged

Conversation

AndrewLipscomb
Copy link
Contributor

Adds a few extra constexpr elements to the schemagen generated code - these elements help allow other compile time libs like Boost Hana to do more operations on the schema (ie: type-based dispatch using a constexpr std::string_view to a boost::hana::type_c<T> for more generic code)

This does modify the generated example schema files (ie: introspection and friends) - I can resubmit this with the updates committed.

@ghost
Copy link

ghost commented Mar 24, 2022

CLA assistant check
All CLA requirements met.

@wravery
Copy link
Contributor

wravery commented Mar 30, 2022

This does modify the generated example schema files (ie: introspection and friends) - I can resubmit this with the updates committed.

Yes, please do push another commit with the regenerated files. That will help me to review the changes since I can see what they look like in context.

@@ -146,6 +146,12 @@ static_assert(graphql::internal::MinorVersion == )cpp"
}
}

headerFile << R"cpp(

using namespace std::literals;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to add a using namespace statement in the header, although this is at least scoped to the generated namespace. Consider leaving this out and wrapping the string literals in explicit std::string_view constructors, like in static_typename instead of appending the sv literal operator.

It looks like this is copied from the sourceFile generation. Could the sourceFile use this instead of declaring its own std::array? Or could the entire mapping function be made constexpr and moved to the header? Then you could also keep the using namespace std::literals; but in the scope of the constexpr function.

@@ -173,6 +179,30 @@ static_assert(graphql::internal::MinorVersion == )cpp"
};

)cpp";
firstValue = true;

headerFile << R"cpp(constexpr std::array<std::string_view, )cpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we don't necessarily include <array> or <string_view> with this header. The PR build failed on Windows saying that those types were undefined. If we want to move this to the header, I think you'll also need to add those #include directives to the headerFile preamble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix up. Apologies - I only have a Linux set of machines to run the tests on here.

@AndrewLipscomb
Copy link
Contributor Author

Sorry it took a while to get back to this - passes Windows checks now, and consolidated the array references to one constexpr declaration

@wravery wravery mentioned this pull request Apr 23, 2022
@wravery wravery merged commit 3fb36ba into microsoft:main Apr 23, 2022
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.

2 participants