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

Bug-Fix for multi-keyword variable-list parsing for properties #52

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PBCOnGit
Copy link

If you try parsing a class containing a variable-list which has more then one keyword like below
class Test { public: int* test1, * test2, * test3; };
The parser will fail because it assumes a single "var-pair" only consists of the name and the seperator.
This approach will fail if tried on a case like above and will cause even more issues if extra modifiers like "const" are used.
The parsers also incorrectly assumes:
int * p1, p2;
boils down to:
int* p1; int* p2;
which is also incorrect since the Cpp-compiler will parse it to:
int* p1; int p2;
The commit tries to address these issues by modifying the component responsible for parsing variable-lists inside CppHeader::_evaluate_property_stack.
The commit also contains tests for the changes and doesn't cause any issues with the original tests.

@virtuald
Copy link
Member

Thanks for the contribution! I'll take a look tonight, but looks like the tests are failing?

@PBCOnGit
Copy link
Author

PBCOnGit commented Nov 20, 2020

Thanks for the contribution! I'll take a look tonight, but looks like the tests are failing?

That is kinda weird to me considering I can run these tests on my machine with no problems.

@virtuald
Copy link
Member

It seems like they're primarily failing on Python 2.7? My initial impression is this looks fine to me, but it doesn't properly handle types with templates in them (eg, Foo<X, Y, Z> x, y, *z;).

I have a local branch that I was working on earlier this week which is rewriting the way we parse function definitions to properly interpret things like std::function<void(int)> which will include code for consuming a template properly. I was hoping to work on it last night, but it looks like it'll be tonight or tomorrow night.

@virtuald
Copy link
Member

I pushed my branch as PR #53 ... if you cherry-pick 286001c that will give you the function to properly consume templates.

@virtuald
Copy link
Member

Taking a harder look at my problems tonight, I remembered the direction I wanted to go with parsing, so I'll be updating my function rework branch with those changes. I think it'll make the most sense to fold the function/property detection together, since it's ambiguous which you're dealing with until a ( or , is encountered (except as part of a template).

As part of that work, I'll fold in your unit tests but probably discard the rest.

@PBCOnGit
Copy link
Author

Taking a harder look at my problems tonight, I remembered the direction I wanted to go with parsing, so I'll be updating my function rework branch with those changes. I think it'll make the most sense to fold the function/property detection together, since it's ambiguous which you're dealing with until a ( or , is encountered (except as part of a template).

As part of that work, I'll fold in your unit tests but probably discard the rest.

Alright, sounds like a good plan.
Thanks for the quick and active responses, even though I still find it weird that the tests are failing and can't reproduce it on my machine.

@virtuald
Copy link
Member

... so I ended up rewriting the entire library?

As a result, cppheaderparser will be abandoned soon in favor of https://github.com/robotpy/cxxheaderparser (which doesn't have this bug and should be a lot more robust). Please check it out, would love any feedback. It's not a drop-in replacement, but it should be much more future proof.

I'm not inclined to spend time fixing this PR for Python 2.7, nor am I inclined to merge this until the tests pass. If you can make the tests pass I'm happy to merge this and push a final release to pypi.

Base automatically changed from master to main January 14, 2021 07:36
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