[NS 1] Add sub-module mounts support + codegen#5167
Open
aasoni wants to merge 1 commit into
Open
Conversation
a5dfaf5 to
c8cac1d
Compare
Comment on lines
+269
to
+273
| bool operator==(const RawModuleMountV10& o) const noexcept { | ||
| if (namespace_ != o.namespace_) return false; | ||
| if (module && o.module) return *module == *o.module; | ||
| return !module && !o.module; | ||
| } |
Contributor
There was a problem hiding this comment.
This fails compilation due to RawModuleDefV10 incomplete at this stage and dereferencing the pointer needs the complete type.
To get through this today we can do a light touch and just check the pointer, it's not as solid as the Rust/TypeScript that does a deeper check but this will compile and we'll have to do the C++ implementation in a future PR anyhow:
Suggested change
| bool operator==(const RawModuleMountV10& o) const noexcept { | |
| if (namespace_ != o.namespace_) return false; | |
| if (module && o.module) return *module == *o.module; | |
| return !module && !o.module; | |
| } | |
| bool operator==(const RawModuleMountV10& o) const noexcept { | |
| return namespace_ == o.namespace_ && module == o.module; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Add a new recursive "mounts" field to add submodules to module def. Handle code generation for each language.
Handle identifiers with "/" or "." in the name to handle namespaced reducers (e.g. lib/reducer) and namespaced tables (lib.table).
Handle code generation for the recursive type. This needed some special handling in code generation for typescript and c++.
Typescript codegen in particular is quite complex as it tries to handle circular dependency generically. C++ on the other hand is a lot simpler because it hard-codes a special handling of the V10 definition but doesn't solve circular dependencies in general.
I would advice against solving circular dependencies in a generic way for C++ however we could consider modifying the typescript code gen to just have special handling for the V10 recursive definition which would simplify the code quite a lot. I went down the rabbit hole of handling this generically and came out on the other side, but if there is strong opinion to keep the codegen code simple, I am happy to revisit and align to the C++ way.
API and ABI breaking changes
The change is purely additive and newer host versions will accept older module defs. However older host versions will not accept new module defs.
Expected complexity level and risk
5 - While this specific PR is maybe a 4, the overall namespace change is definitely 5. This is a pretty significant change. It's a large diff which touches the module def and changes code that hasn't been touched in a long time (e.g. Identifier).
Testing
Beyond the rust tests defined in this PR, the following tests were done on the full PR sequence once the entire namespace feature was implemented for typescript:
Feature Test Checklist
Module:
Client
CLI
Migration
Commit Log