-
Notifications
You must be signed in to change notification settings - Fork 680
Add C++ Bindings #3544
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
base: master
Are you sure you want to change the base?
Add C++ Bindings #3544
Conversation
8a32cdb to
7b372b7
Compare
…_exports to carry defaults
… as exceptions are not available for C++ yet
…r to how Views worked with errors + added a version of IterBuf for C++ to start to reduce allocations
7b372b7 to
fa45b54
Compare
# Conflicts: # templates/templates-list.json
crates/bindings-cpp/tests/type-isolation-test/test_modules/test.o
Outdated
Show resolved
Hide resolved
crates/bindings-cpp/include/spacetimedb/bsatn/algebraic_type.h.backup
Outdated
Show resolved
Hide resolved
Thanks! Co-authored-by: Ryan <[email protected]> Signed-off-by: Jason Larabie <[email protected]>
jdetter
left a comment
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.
Some comments/questions to get started here. There is a lot to review here so I'll do another pass later today.
The main thing I would bring up is that we are referring to this as the "C++ SDK" which I think we should rebrand to the "C++ bindings". My reason is that the other bindings libraries all refer to themselves as bindings, not an SDK. We typically use the nomenclature "SDK" to mean the client SDKs, not the module side bindings.
I fear that if we start referring to this as our C++ SDK this will both confuse users and confuse the LLM training especially if we add a C++ client SDK in the future. If you want feedback from the team on this it might be a good topic to bring up during standup.
Let me know when you're ready for another pass, I'm really excited that this seems like it's working so far and I'm excited for the future with C++26! 🙂
| @@ -0,0 +1,1290 @@ | |||
| # SpacetimeDB C++ Module Reference | |||
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.
does this become our API reference doc?
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.
I think I'll work on a new version that matches with how the others are laid out, this was a hold over from Arvikasoft's work that I've maintained for now to give us a starting point.
| @@ -0,0 +1,293 @@ | |||
| # SpacetimeDB C++ Module Quickstart | |||
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.
This becomes our quickstart chat example in the docs, right?
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.
Same as the REFERENCE I want to get this matching the rest of our docs and this file might go away so we only maintain one location for documentation.
crates/bindings-cpp/QUICKSTART.md
Outdated
|
|
||
| SpacetimeDB C++ uses a unique accessor pattern: | ||
| - `ctx.db[table_name]` - Access table for iteration and basic operations | ||
| - `ctx.db[table_field]` - Access indexed fields for optimized operations (generated symbol named `tableName_fieldName`) |
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.
This seems a bit weird, shouldn't it be something like this instead:
ctx.db[table_name].table_field
What if 2 tables have the same indexed field name?
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.
Ahh yeah this is still confusing, it is ctx.db[table_name_and_field_name] with how the accessors get generated. Maybe I change it to?
- `ctx.db[table]` - Access table for iteration and basic operations, eg. ctx.db[user]
- `ctx.db[table_field]` - Access indexed fields for optimized operations, eg. ctx.db[user_id]
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.
I've made the change and going to leave this comment open as it might not be good enough yet
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.
If the goal is to simply clarify how it currently works, my personal preference towards camelCase:
- `ctx.db[tableName]` - Access table for iteration and basic operations, eg. ctx.db[user]
- `ctx.db[tableName_fieldName]` - Access indexed fields for optimized operations, eg. ctx.db[user_id]
This is because it allows it to be clear that we are specifically referring to the name of a given table/field, while also not adding any additional special characters, like underscore, that is present in snake_case. The extra underscore get confusing when those are also used as the table to field delimiters.
Co-authored-by: John Detter <[email protected]> Signed-off-by: Jason Larabie <[email protected]>
Co-authored-by: John Detter <[email protected]> Signed-off-by: Jason Larabie <[email protected]>
…abs/SpacetimeDB into jlarabie/bindings-cpp
Description of Changes
This adds C++ server bindings (/crate/bindings-cpp) to allow writing C++ 20 modules.
API and ABI breaking changes
None
Expected complexity level and risk
2 - Doesn't heavily impact any other areas but is complex macro C++ structure to support a similar developer experience, did have a small impact on init command
Testing