Sdk #78
Sdk #78
Conversation
There was a problem hiding this comment.
Okay, first of all, thanks for the contributions.
I've looked at several things and a couple of high level points:
- please adapt the sdk to the google c++ coding style guide a couple "mistakes" I've seen a lot:
- member variables should have a trailing
_ - trailing return types
auto x(...) const noexcept -> int - I very often saw a space between function name and opening parenthesis
/test (...)/test(...) - remove
using namespace std - Occasionally I have also occasionally hungarian naming notation, please replace these with descriptive names
- The is
.clang-tidyand.clang-formatfile which enforce the style.
- member variables should have a trailing
- Try make things
constexpr: I think we should be able to precompile a lot of things in the SDK. - Duplication of work between the SDK and the runtime: I've often seen you introduce member variables that do some kind of bookkeeping. The runtime already does a lot of this bookkeeping, try to take advantage of this. Otherwise, it will be very hard to implement new runtime features (like mutations).
- I would remove the images and dot files from the examples.
| @@ -0,0 +1,17 @@ | |||
| #include "Config-a.hh" | |||
There was a problem hiding this comment.
make a folder for all the sdk stuff:
/examples/sdk/SrcSinkFanout/ConfigA/ConfigA.cc
|
|
||
| void MainReactor::construction() { | ||
|
|
||
| cout << "Construction Main\n"; |
There was a problem hiding this comment.
did you overload cout? or did I miss the using namespace std ?
| @@ -0,0 +1,44 @@ | |||
| digraph { | |||
There was a problem hiding this comment.
I think this file should not be added.
There was a problem hiding this comment.
I think this file should not be added.
| cxxopts::ParseResult result{}; | ||
| bool parse_error{false}; | ||
| try { | ||
| result = options.parse(argc, argv); |
| reactor::ShutdownTrigger shutdown{"shutdown", this}; | ||
|
|
||
| private: | ||
| size_t bank_index_ = 0; |
There was a problem hiding this comment.
I see so this is so every reactor knows what index he has inside a bank? (Like the example I gave couple of months ago?)
|
|
||
| private: | ||
| size_t bank_index_ = 0; | ||
| SystemParameterBase *p_param = nullptr; |
There was a problem hiding this comment.
/p_param/system_parameters_/
| private: | ||
| size_t bank_index_ = 0; | ||
| SystemParameterBase *p_param = nullptr; | ||
| Environment *env{nullptr}; |
There was a problem hiding this comment.
This is duplicate. You can get it via the environment() function.
/env/env_/ (https://google.github.io/styleguide/cppguide.html)
| Reactor *parent{nullptr}; | ||
| std::string current_reaction_name; | ||
| std::unordered_map<std::string, std::shared_ptr<BaseTrigger>> reaction_map; | ||
| int priority = 1; |
There was a problem hiding this comment.
what does the priority field?
| std::string current_reaction_name; | ||
| std::unordered_map<std::string, std::shared_ptr<BaseTrigger>> reaction_map; | ||
| int priority = 1; | ||
| std::set<Reactor*> child_reactors; |
There was a problem hiding this comment.
inside the Reactor class you have the reactor_ field which keeps track of the child reactors.
There was a problem hiding this comment.
Besides the stylistic issues as noted by Tassilo, I currently see three major hurdles for accepting this PR.
- I am currently not convinced that an SDK fits in the scope of this repository. As of now, the purpose of this repository is to provide a reactor runtime implementation supporting lf-lang/lingua-franca where it is integrated as a submodule. As the proposed SDK is meant as an alternative to LF, it does not serve the current purpose of the repository. I am open to a discussion around changing the scope of the repository. However, I want to make sure that such a change happens deliberately and with good reasons. Also, a change in scope implies a change in the relationship with lf-lang/lingua-franca that would need to be carried by the community.
- The lack of mechanisms to prevent the user from violating the reactor semantics. While the SDK (like the existing runtime API) provides one correct way for expressing reactor programs, there are also many more wrong ways that the user can choose. There are no guardrails that nudge the user in the right direction, or even better, prevent the users from violating the reactor semantics. While there are limitations to how far one can go with this, I strongly believe that a good measure of protection techniques is required for a reactor SDK to be useful and successful.
- The lack of a domain boundary. In LF, the code generator establishes a layer of indirection that can be used to shield user code from changes in the runtime code. The SDK in this PR, however, fully exposes the underlying runtime types and their API. This creates a tight coupling between the runtime code and the user code. This coupling makes it harder to evolve and maintain the runtime. To me, this seems like the opposite of what we want moving forward. Establishing a clear domain boundary would both shield the user from internal runtime changes, and shield the runtime from having to maintain too many user-facing contracts.
…ports in case the width don't match
What this SDK provides
Features missing so far, work in progress
Building
cd reactor-cppmkdir build && cd build && cmake ..make -j && make installExamples and Tests
examples and test folders have use cases that help understand the concepts.
Immediate next item