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

rust-python-parser tidy up #3503

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

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Feb 9, 2025

  1. Tests move into a different compilation unit from logic
  2. Put the parseImports function into the same statib library as the rust objects.

@hoodmane hoodmane requested review from a team as code owners February 9, 2025 08:45
namespace workerd {
namespace rust {
namespace python_parser {
::rust::Vec<::rust::String> get_imports(::rust::Slice<::rust::Str const> sources);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love declaring this manually, but at least C++ name mangling means there is a very high probability of a link error rather than generating a bad executable if the declaration is wrong. The problem is this is declared in workerd/rust/python-parser/lib.rs.h which is generated by wd_rust_crate but that is a dependent of the rule that compiles cxx-bridge.c++. This probably means I'm doing something wrong in Bazel? cc @mikea

@hoodmane hoodmane force-pushed the hoodmane/rust-python-parser-tidy branch 2 times, most recently from d086e36 to b968057 Compare February 10, 2025 11:13
@hoodmane hoodmane requested a review from mikea February 10, 2025 12:12
@hoodmane hoodmane force-pushed the hoodmane/rust-python-parser-tidy branch from b968057 to c1a11df Compare February 10, 2025 12:19
1. Tests move into a different compilation unit from logic
2. Put the `parseImports` function into the same statib library as
   the rust objects.
@hoodmane hoodmane force-pushed the hoodmane/rust-python-parser-tidy branch from c1a11df to e2adb8f Compare February 10, 2025 12:20
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