Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
PBCOnGit wants to merge1 commit intorobotpy:main
base:main
Choose a base branch
Loading
fromPBCOnGit:master

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 commentedNov 20, 2020
edited
Loading

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 likestd::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-pick286001c 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 ofhttps://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 frommaster tomainJanuary 14, 2021 07:36
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PBCOnGit@virtuald

[8]ページ先頭

©2009-2025 Movatter.jp