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

Adding cli option to remove interfaces constraints on records. #156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jferreyra-sc
Copy link
Contributor

@jferreyra-sc jferreyra-sc commented Dec 15, 2023

Context

The motivation for this change is that often you want to pass a set of interfaces in a record instead of different parameters in a function.
This helps for the code to be more scalable/maintainable as we can separate the creation of the parameters from the calling and handling of them.

Example:

class Instance /* ... */

struct Callbacks {
   std::nn_shared_pointer<TasteDescriptor> tasteDescriptor;
   std::nn_shared_pointer<CostDescriptor> costsDescriptor;
   std::nn_shared_pointer<LocalizationDescriptor> localizationDescriptor;      
   std::shared_pointer<LogDescriptor> logDescriptor;      /* optional */
};

class SDK {
    std::shared_ptr<Instance> createInstance( const Callbacks& callbacks );
};

Then the caller can do:

auto instance = SDK::createInstance( createAndConfigureCallbacks() );

Please notice that you were able to pass optional interfaces using the optional<> wrapper (which was properly interpreted and generated), and some users made used of a notnull<> extension to pass not null interfaces as a workaround. However, the extension workaround has a few weakness that are better to avoided (more boilerplate code, less readable code, some tricky implementation of this notnull extension).

Changes

This PR adds an option to remove the constraint, though, we should consider to directly always enable it as I see no reason to disallow them. My rationale is the following (please check for any wrong assumptions):

Records only go from one language to another during function calls, function calls can have parameters or return values that are interfaces. There is no environments where records are transmitted in a situation where interfaces are not supported.

Remote invocations are not a problem, for the same reasons explained above.

I imagine that if we use records for serialization to permanent storage, then interfaces wouldn't fly, but I believe the correct behavior would be to only restrict interfaces for this specific case, and not for the other cases.
On the same line, I guess DataRefs disallowed serialization to permanent storage already, so this case should not be concerning.

Admins, please let me know if you prefer the change as it is or if you prefer to always relax it.

Testing

I have tested the changes by extending the examples to use records holding interfaces instead of passing them directly. You can compare the diff to this branch here.

Both Android and iOS examples built and executed correctly.

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.

1 participant