search

robotpy / robotpy-cppheaderparser

DEPRECATED: use cxxheaderparser instead
Other
123 stars 39 forks source link

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

Open PBCOnGit opened 3 years ago

PBCOnGit commented 3 years ago

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 commented 3 years ago

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

PBCOnGit commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

virtuald commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

... 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.