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

[#491] Propagate error strings from iceoryx2 via CXX API #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

orecham
Copy link
Contributor

@orecham orecham commented Nov 8, 2024

Notes for Reviewer

Adds C/C++ API to retrieve pointer to string describing iceoryx2 errors.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Closes #491

C_IS_BEING_CREATED_BY_ANOTHER_INSTANCE,
C_OLD_CONNECTION_STILL_ACTIVE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error doesn't exist in the iceoryx2 error enum.

@orecham orecham force-pushed the iox2-491-cxx-api-for-error-description-string branch from d77726c to ab23edf Compare November 8, 2024 10:47
@orecham orecham self-assigned this Nov 8, 2024
@orecham orecham requested review from elBoberido and elfenpiff and removed request for elBoberido November 8, 2024 10:49
Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

Good work and congratulations to the proc macro!

The only thing here is, that I would follow the iceoryx type conversion approach here and use from<EnumType, const char*> since it is a conversion from enum to a string. In this way it is nicely consistent.

@@ -25,6 +25,9 @@ enum class ConfigCreationError : uint8_t {
/// Parts of the config file could not be deserialized. Indicates some kind of syntax error.
UnableToDeserializeContents
};

auto error_string(const iox2::ConfigCreationError& error) -> const char*;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use here the From approach like it is done here: https://github.com/eclipse-iceoryx/iceoryx2/blob/main/iceoryx2-ffi/cxx/include/iox2/enum_translation.hpp

Since it is a conversion from enum iox2::ConfigCreationError to const char*.

This applies to all error_string() free functions.


namespace iox2 {

auto error_string(const iox2::ConfigCreationError& error) -> const char* {
Copy link
Contributor

Choose a reason for hiding this comment

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

All those implementation should be changed to the format:

template <>
constexpr auto from<iox2::ConfigCreationError, const char *>(const iox2::ConfigCreationError value) noexcept -> const char * {
  return iox2_config_creation_error_string(iox::into<iox2_config_creation_error_e>(value));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also rename the file into enum_string.cpp. It makes sense to have this conversion also for non error enums like ServiceType for instance.

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.

Provide error description strings via C/C++ API
2 participants