-
Notifications
You must be signed in to change notification settings - Fork 502
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
Sshirokov/336 transport #340
Sshirokov/336 transport #340
Conversation
- added `ITransport::makeMessage[Rx|Tx]Session` methods - added `ITransport::make[Request|Response][Rx|Tx]Session` methods Also: - added `libcyphal::UniquePtr` & `libcyphal::Expected` - added`transport::AnyError`
…essageRxTransfre` and `ServiceRxTransfer` types.
public: | ||
Factory() = delete; | ||
|
||
CETL_NODISCARD static inline Expected<UniquePtr<ICanTransport>, FactoryError> make( |
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.
Why can't this be a freestanding function?
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.
My rationale was to avoid freestanding functions (which for my taste a bit "c-style"), but instead you know "encapsulate" them into some sort of entity (like this Factory
). Also, IMHO if we apply such pattern everywhere then it will be easier for users to guess where to find a factory method, like always <correct_namespace>::Factory::make(...)
something. But in general I'm totally good with essentially the same <correct_namespace>::makeSomething(...)
approach.
Probably it's again my Swift habits - there are no namespaces in Swift, so any freestanding function is GLOBAL one, which is considered as bad design - so you nest such functions in some (sub)type as static one.
Please let me know if my rationale is not applicable for c++, or there are reasons to specifically go with freestanding alternative. Thanks!
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.
Yeah, I don't think it applies here; we should rather keep things simple and use a freestanding function.
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.
will fix this in the next pr
public: | ||
Factory() = delete; | ||
|
||
CETL_NODISCARD static inline Expected<UniquePtr<ICanTransport>, FactoryError> make( |
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.
Yeah, I don't think it applies here; we should rather keep things simple and use a freestanding function.
ITransport
,ICanTransport
&IUpdTransport
PayloadFragment
now immutable;FragmentBuffer
is mutable span.