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

Add support for decltypes #75

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

Conversation

kdkavanagh
Copy link

Adds support for decltypes in return types and method parameter types. Previously the parser was thrown off by the inclusion of the extra parenthesis used with decltype()

@virtuald
Copy link
Member

virtuald commented Mar 4, 2022

Thanks for the contribution! It's likely that I won't have time to review until Saturday evening, but I'll do my best.

@virtuald
Copy link
Member

I definitely forgot about this! I'm traveling, but will add it to my calendar for when I get home. There's nothing obviously wrong as far as I can see, but I have a larger corpus of stuff to run it against at home.

@kdkavanagh
Copy link
Author

Hey @virtuald, any updates on this?

@virtuald
Copy link
Member

virtuald commented Apr 6, 2022

... definitely forgot again. Adding it back to my calendar, I don't have anything urgent planned tonight so I should be able to do it then.

@virtuald
Copy link
Member

virtuald commented Apr 7, 2022

This PR failed to parse one of the core RobotPy libraries that we use CppHeaderParser for. I'll dig in to see if I can isolate it.

Copy link
Member

@virtuald virtuald left a comment

Choose a reason for hiding this comment

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

Ok, this is pretty dumb, but bear with me.

Your PR not only added support for decltypes, but also changed out the way that functions/etc are formatted. Unfortunately, our build tool robotpy-build currently (stupidly) depends on the specific formatting of types to match parameters and such... for example:

- wpi::Logger&, std::function<void ( wpi::span<const uint8_t> data )>, double, std::string_view
+ wpi::Logger&, std::function<void(wpi::span<const uint8_t> data)>, double, std::string_view:

Since our build tool is the only reason I maintain this library, I can't accept your PR with the output formatting changes at this time.

The Right Fix for this is to make robotpy-build ignore the spacing when matching types. However, I don't really want to mess with that in the middle of robotics season (which ends at the end of April) in case we need to push updates. And anyways, long term, I want to abandon cppheaderparser in favor of cxxheaderparser (have you tried it yet? it handles decltypes) and refactor robotpy-build... but that's still a bit farther off than I would like.

@kdkavanagh
Copy link
Author

Understood. I may be able to back out that change specifically.

Wasn't aware of cxxheaderparser - Fully supportive of moving off this codebase :). Do you feel it has at least close to feature parity with this library? If not, any idea of what's still missing?

FWIW, one thing we do in our codegen CI is run all generated code thru clang-format before comparing with expectations to ensure that formatting itself doesnt cause issues. Not sure if relevant to your stuff, just an unsolicited best practice we've found.

@virtuald
Copy link
Member

virtuald commented Apr 8, 2022

cxxheaderparser does many of the same things as this (and is better at handing some C++11+ features), but intentionally doesn't do some of the weird ctypes/type resolving things that CppHeaderParser does.

It should be mostly feature complete, but some ambiguous constructs aren't quite supported yet. If you take a look at the tests, you can see what things are currently supported (and what the output looks like): https://github.com/robotpy/cxxheaderparser/tree/main/tests

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