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

Move c++ parsing code from slang-cpp-extractor to static library #5675

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

laurooyen
Copy link

@laurooyen laurooyen commented Nov 25, 2024

As part of #5565, this is the initial work on extracting c++ parsing code from slang-cpp-extractor to a separate static library. This will allow me to reuse the parser for my work on the language bindings generator. The current slang-cpp-extractor and newly created slang-cpp-parser can still use some refactoring but I feel like that is better left for future PRs once I know how much additional code can be shared with the binding generator.

Please ensure especially my changes to the CMake file look good, I'm not too familiar with it.

@laurooyen laurooyen requested a review from a team as a code owner November 25, 2024 19:39
@laurooyen
Copy link
Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@csyonghe
Copy link
Collaborator

@expipiplus1 can you please take a look?

Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

Is there a related issue for this PR?
I am not sure what problem this PR is trying to solve.
I am also not sure if it is safe to rename from CppExtract to CppParse without causing troubles on the users who might be already using them.

Comment on lines +17 to +22
#include "../tools/slang-cpp-parser/diagnostics.h"
#include "../tools/slang-cpp-parser/file-util.h"
#include "../tools/slang-cpp-parser/node.h"
#include "../tools/slang-cpp-parser/options.h"
#include "../tools/slang-cpp-parser/parser.h"
#include "../tools/slang-cpp-parser/unit-test.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid using "../" here?
We already have a few but we should try to remove them not adding them.

@laurooyen
Copy link
Author

This was discussed in #5565. My bad for not including it, I updated the PR description. It allows us to reuse the C++ parser code for generating C (and other) language bindings by parsing slang.h. I'd like to work on that in future PRs.

I didn't see any harm in renaming parts of CppExtract to CppParse because it felt like an internal API but I could be wrong.

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.

5 participants