- Notifications
You must be signed in to change notification settings - Fork40
Add support for decltypes#75
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the contribution! It's likely that I won't have time to review until Saturday evening, but I'll do my best. |
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. |
Hey@virtuald, any updates on this? |
... 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. |
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. |
There was a problem hiding this 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 toolrobotpy-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.
Understood. Imay 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. |
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 |
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()