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

First template instanciation full argument parsing #99

Closed
wants to merge 0 commits into from

Conversation

Firefly35
Copy link
Contributor

Hi Jonathan,
I wrote a first version for template instantiation full argument parsing in cppast.
I propose the code for review.
Note : I didn't add the proposal to have the choice between exposed or unexposed mode for arguments in this version.

@Firefly35
Copy link
Contributor Author

Firefly35 commented Jan 16, 2020

I modified azure-pipelines to add the needed jobs/steps to build with the new -DCPPAST_TEMPLATE_FULLARGUMENTSPARSING=ON option
Note : I had to comment some parsing tests that need a rewrite to handle exposed arguments

Copy link
Collaborator

@foonathan foonathan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing it, appreciate it!

While I appreciate that you don't want to break existing users by guarding the feature behind a compile-time option, I prefer to be more aggressive with useful improvements such as this one. As such, I'd suggest you enable the option by default (with the intent to remove it eventually). I also don't think it is necessary to keep testing the old behavior.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/libclang/type_parser.cpp Outdated Show resolved Hide resolved
auto decl = clang_getTypeDeclaration(type);
auto count = clang_Type_getNumTemplateArguments(clang_getCursorType(decl));
for (unsigned int index = 0 ; static_cast<int>(index) < count ; index++) {
CXType argType = clang_Type_getTemplateArgumentAsType(type,index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about non-type template parameters or template template parameters?

src/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
builder.add_unexposed_arguments("void");
REQUIRE(equal_types(idx, alias.type_alias().underlying_type(), *builder.finish()));
#else
// need to write the code for full argument parsing using add_argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by this comment?

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 didn't have the needed time to write the test for full argument parsing.

@foonathan
Copy link
Collaborator

Sorry for the delay. The build and CI setup looks good, I'll merge it once, you've found the time to update tests and figure out what happens when you have non-type template parameters in there etc.

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