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

Add edge definition API #437

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Add edge definition API #437

merged 1 commit into from
Oct 7, 2024

Conversation

ivan-cukic
Copy link
Contributor

No description provided.

@ivan-cukic ivan-cukic linked an issue Oct 6, 2024 that may be closed by this pull request
Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, 2 small comments/comprehension questions.

core/include/gnuradio-4.0/Port.hpp Outdated Show resolved Hide resolved
@@ -568,6 +565,24 @@ struct Port {

[[nodiscard]] constexpr static bool isOptional() noexcept { return kIsOptional; }

[[nodiscard]] constexpr std::size_t nReaders() const noexcept {
if constexpr (kIsInput) {
return -1UZ;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was wondering if this should/could be a runtime or compile time error(the function only defined for Outputs)? Compile time error probably won't work because of the virtual interface and use in the runtime API, runtime error does not work because of noexcept. Would it be useful to have an assert(false)?

Functionally I think this is good, but it kind of communicates that the user will have to check the number of readers against MAX_SIZE_T which in my understanding of the code should never happen -> it's e.g. sufficient to check for edge.readers > 0 to check for presence of readers.

Ties a bit into the bigger discussion on how we handle errors in general, so this is probably not really actionable right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert(false) would be ok, but I'm not sure we should do it. As this is a general/unified interface, I'm more for communicating the error as -1 - the user is not going to need to first check the type of the port and then call the functions, just call and ignore -1 if that is the result.

if constexpr (kIsInput) {
return _ioHandler.buffer().n_writers();
} else {
return -1UZ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why, -1UZ? Why not 0, or, if this is considered an error, add a failing static_assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reply to Alex above

Signed-off-by: Ivan Čukić <[email protected]>
@wirew0rm wirew0rm merged commit 2439631 into main Oct 7, 2024
9 of 11 checks passed
@wirew0rm wirew0rm deleted the ivan/edge-api branch October 7, 2024 13:57
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.

[2SP;3SP] Add C++ and message-based Edge API
3 participants