Fix convert edge cases #2135

Dennis40816 · 2023-12-15T07:31:16Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

I'm not sure the values of new IOX_2055_WORKAROUNDs are proper or not. Please check them.

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes 'convert' is broken for edge cases #2055.

codecov · 2023-12-15T08:02:02Z

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (154b5a5) 80.22% compared to head (e02d08b) 80.12%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2135      +/-   ##
==========================================
- Coverage   80.22%   80.12%   -0.11%     
==========================================
  Files         418      418              
  Lines       16172    16218      +46     
  Branches     2255     2251       -4     
==========================================
+ Hits        12974    12994      +20     
- Misses       2399     2420      +21     
- Partials      799      804       +5

Flag	Coverage Δ
unittests	`79.91% <76.49%> (-0.11%)`	⬇️
unittests_timing	`15.21% <0.00%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
iceoryx_hoofs/cli/include/iox/cli/arguments.inl	`58.82% <100.00%> (ø)`
...x_hoofs/utility/include/iox/std_string_support.hpp	`100.00% <100.00%> (ø)`
...eoryx_posh/internal/runtime/ipc_interface_base.hpp	`100.00% <ø> (ø)`
...yx_posh/internal/runtime/ipc_runtime_interface.hpp	`100.00% <ø> (ø)`
iceoryx_posh/source/popo/client_options.cpp	`100.00% <100.00%> (ø)`
iceoryx_posh/source/popo/server_options.cpp	`100.00% <100.00%> (ø)`
iceoryx_posh/source/popo/subscriber_options.cpp	`93.75% <100.00%> (+0.41%)`	⬆️
iceoryx_posh/source/roudi/process_manager.cpp	`64.61% <100.00%> (-0.09%)`	⬇️
iceoryx_posh/source/roudi/roudi.cpp	`66.28% <100.00%> (+0.65%)`	⬆️
...ceoryx_posh/source/roudi/roudi_cmd_line_parser.cpp	`100.00% <100.00%> (ø)`
... and 7 more

... and 4 files with indirect coverage changes

elBoberido · 2023-12-16T19:16:03Z

Thanks for the PR. I'll have a closer look tomorrow at the changes but I'm quite happy that the out parameters are soon a thing of the past :)

elBoberido

I did not yet fully review the tests and convert.inl. Just had a remark regarding the newly introduced alwaysSuccess. Will finish the review tomorrow.

I think we (and always when I say we I actually mean you :) ) also need to add tests for the corner cases this PR fixes. If you think it will blow the PR up too much, it could also be done in a follow up PR.

iceoryx_dust/cli/include/iox/cli/arguments.inl

iceoryx_dust/utility/include/iox/detail/serialization.inl

iceoryx_examples/iceperf/main_follower.cpp

iceoryx_posh/source/roudi/roudi.cpp

iceoryx_dust/utility/include/iox/detail/serialization.inl

iceoryx_posh/source/runtime/ipc_interface_base.cpp

iceoryx_posh/source/runtime/ipc_runtime_interface.cpp

iceoryx_posh/source/roudi/port_manager.cpp

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

tools/introspection/source/introspection_app.cpp

Rename `fromString` to `from_string`. Note that the name of test cases are not included. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

Remove dest for numeric conversion. Now, the part of `for_string` will return iox::optional. Also, we introduce `validate_return_value` and `check_edge_case` to simplify the code of checking. The former can be used to return an iox::optional object while the latter is used in the former to check whether any conversion failure happened. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

@todo

The from_string API has been updated. It now returns an iox::optional of the specified target value type, and the dest parameter has been removed. Subsequently, functions that utilize from_string have been restructured to accommodate the new calling format. Some of these functions now require default values for the value_or method, but I am uncertain whether these default values are appropriate. A new function named forceOkReturnValue has been introduced (the name may be subject to change later). This function addresses a specific issue encountered with uint64_t cases. Previously, we used failureReturnValue(ULLONG_MAX), which incorrectly flagged the valid conversion case of "18446744073709551615" as a failure. With forceOkReturnValue, the condition call.has_error() should consistently return false for this scenario. The accuracy of the conversion result is now assessed within the check_edge_case function. As a note, areas marked with @todo iox-eclipse-iceoryx#2055 still require modifications, most of which pertain to documentation. These will be addressed in the upcoming commit. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

In iceoryx_posh/source/runtime/posh_runtime_impl.cpp, convert::from_string are used to convert the segment id and offset. However, the conversion failure might occur. Therefore, an new error type: `SEGMENT_ID_OR_OFFSET_CONVERSION_FAILURE` are introduced to be utilized as an error return value, reminding user a conversion failure happened in posh runtime. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

elBoberido

Looks good. Mostly nitpicks and a few new tests for the edge cases needs to be written :)

iceoryx_posh/source/runtime/ipc_interface_base.cpp

iceoryx_hoofs/utility/include/iox/detail/convert.hpp

iceoryx_hoofs/utility/include/iox/detail/convert.inl

iceoryx_hoofs/test/moduletests/test_utility_convert.cpp

Change the template args of specialization of `from_string` from length fixed type to basic type. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

Function named `convert_id_and_offset` was created. Signed-off-by: Dennis Liu <[email protected]>

Now, `is_valid_errno` will also print the string that cause failure. Signed-off-by: Dennis Liu <[email protected]>

Dennis40816 · 2023-12-20T18:18:24Z

It seems that there are some errors occurring during the execution of all_test. I will take a closer look at it tomorrow.

elBoberido · 2023-12-20T22:31:48Z

It seems that there are some errors occurring during the execution of all_test. I will take a closer look at it tomorrow.

I think you forgot long and unsigned long

Signed-off-by: Dennis Liu <[email protected]>

- Add long and unsigned long specialization for from_string. - Remove deprecated `is_iox_string` and `GetCapacity`. - Move the specialization of std::string to `iox/std_string_support.hpp`. - Replace `enable_if_t` by `if constexpr` branch. Signed-off-by: Dennis Liu <[email protected]>

Signed-off-by: Dennis Liu <[email protected]>

- Add a todo in `is_valid_errno`. - Clean `errno` before leaving from `evaluate return value`. Signed-off-by: Dennis Liu <[email protected]>

Modify the target function of the `IOX_POSIX_CALL` invocation, and for those that might encounter an edge case error due to the invocation of `failureReturnValue`, replace it with `alwaysSuccess`. Specifically, this applies to unsigned/signed types: `long long`, `long`, and `int` (for 32-bit systems). Please note that we assume `int` is always 4 bytes, and `long long` is definitively 8 bytes. Signed-off-by: Dennis Liu <[email protected]>

Dennis40816 · 2023-12-21T13:36:52Z

If there are no other issues with the aforementioned modifications, I think we can start adding some edge cases for testing purposes.

elBoberido · 2023-12-21T14:25:05Z

If there are no other issues with the aforementioned modifications, I think we can start adding some edge cases for testing purposes.

I did not yet look at the code but I don't expect any issues. Feel free to start whenever it pleases you :)

elBoberido

Sorry, I gave you bad advice with suppressErrorMessagesForErrnos. With ignoreErrnos the alwaysSuccess method can be removed.

Except that there are just a few minor things like not having to explicitely cache the errno because it is already done in the result of the posix call.

iceoryx_hoofs/utility/include/iox/detail/convert.inl

iceoryx_hoofs/utility/include/iox/detail/convert.hpp

iceoryx_hoofs/utility/include/iox/detail/convert.inl

iceoryx_posh/source/runtime/ipc_interface_base.cpp

Signed-off-by: Dennis Liu <[email protected]>

iceoryx_posh/source/runtime/ipc_interface_base.cpp

iceoryx_hoofs/utility/include/iox/detail/convert.inl

elBoberido

I think now is everything covered. The check for signaling NAN can be removed. Sorry for that :)

iceoryx_hoofs/utility/include/iox/detail/convert.inl

Signed-off-by: Dennis Liu <[email protected]>

elBoberido

Except for some additional tests to check for the edge cases this looks now great.

From my point of view the new tests could also be added in a new PR if you think this one is already overloaded. I leave it to you.

Dennis40816 · 2024-01-02T18:19:18Z

Thank you for your review. I would like to create a new PR. Our conversation on this PR has already been quite lengthy, and I believe there will be more to discuss when testing edge cases. Therefore, I think we should start a new PR focused on conducting tests for edge cases :)

elBoberido · 2024-01-02T19:36:31Z

Merging this PR. All the old tests pass and additional tests for corner cases will be added in a separate PR

elBoberido reviewed Dec 19, 2023

View reviewed changes

Dennis40816 added 15 commits December 19, 2023 13:29

iox-eclipse-iceoryx#2055 Rename fromString to from_string

6196d41

Rename `fromString` to `from_string`. Note that the name of test cases are not included. Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Remove convert::stringIsNumber related func

4016555

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Add local variable end_ptr

60b7c79

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Add suppressErrorMessagesForErrnos

80d7830

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Rename to evaluate_return_value

1082cc5

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Add comment for functions

aaeb4a9

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Try to fix macos sanitizer problem

c171c7f

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Try to fix uninitialize error

31cdc64

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Fix MacOS and arm-none-eabi-gcc API

0a3eb9f

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Initialize variables

734ac26

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Remove unsed variables

c486577

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Refactor the usage of from_string - 1

809dddc

Signed-off-by: Dennis Liu <[email protected]>

Dennis40816 force-pushed the iox-2055-fix-convert-edge-cases branch from 1c377a8 to 809dddc Compare December 19, 2023 08:26

elBoberido reviewed Dec 19, 2023

View reviewed changes

Dennis40816 added 4 commits December 20, 2023 22:40

iox-eclipse-iceoryx#2055 Change the from_string template arg

c9ffbf2

Change the template args of specialization of `from_string` from length fixed type to basic type. Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Refactor roudi_cmd_line_parser.cpp

ee6121e

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Refactor id and offset conversion

dd422c2

Function named `convert_id_and_offset` was created. Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Improve error msg

be1ab23

Now, `is_valid_errno` will also print the string that cause failure. Signed-off-by: Dennis Liu <[email protected]>

Dennis40816 added 4 commits December 21, 2023 11:07

iox-eclipse-iceoryx#2055 Use type alias to make code more concise

61232c1

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Replace post-increment with pre-increment

79cac77

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Rename to SourceType and source_val

5ab15f5

Signed-off-by: Dennis Liu <[email protected]>

Dennis40816 added 3 commits December 21, 2023 16:04

iox-eclipse-iceoryx#2055 Use assertion for has_value check

640c010

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Add a todo in is_valid_errno

e83b76d

- Add a todo in `is_valid_errno`. - Clean `errno` before leaving from `evaluate return value`. Signed-off-by: Dennis Liu <[email protected]>

elBoberido reviewed Dec 22, 2023

View reviewed changes

Dennis40816 added 4 commits December 24, 2023 21:00

iox-eclipse-iceoryx#2055 Remove alwaysSuccess

3defd1e

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Remove unused comment code

809b9a9

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Remove errno_cache from 'evaluate_return_value'

4706afe

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Add 'is_signaling_nan'

3801cf5

Signed-off-by: Dennis Liu <[email protected]>

elBoberido reviewed Dec 27, 2023

View reviewed changes

iceoryx_posh/source/runtime/ipc_interface_base.cpp Outdated Show resolved Hide resolved

iceoryx_hoofs/utility/include/iox/detail/convert.inl Outdated Show resolved Hide resolved

elBoberido reviewed Dec 27, 2023

View reviewed changes

iceoryx_hoofs/utility/include/iox/detail/convert.inl Outdated Show resolved Hide resolved

iceoryx_hoofs/utility/include/iox/detail/convert.inl Outdated Show resolved Hide resolved

iceoryx_hoofs/utility/include/iox/detail/convert.inl Outdated Show resolved Hide resolved

Dennis40816 added 3 commits December 31, 2023 21:40

iox-eclipse-iceoryx#2055 Replace 'Destination' with 'TargetType'

d3f692a

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Remove 'is_signaling_nan' and 'errno = 0'

a24be4a

Signed-off-by: Dennis Liu <[email protected]>

iox-eclipse-iceoryx#2055 Simplify underlying type by alias

e02d08b

Signed-off-by: Dennis Liu <[email protected]>

elBoberido approved these changes Jan 2, 2024

View reviewed changes

elBoberido merged commit fe3ef2e into eclipse-iceoryx:master Jan 2, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix convert edge cases #2135

Fix convert edge cases #2135

Dennis40816 commented Dec 15, 2023 •

edited by elBoberido

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading

elBoberido commented Dec 16, 2023

elBoberido left a comment

elBoberido left a comment

Dennis40816 commented Dec 20, 2023

elBoberido commented Dec 20, 2023 •

edited

Loading

Dennis40816 commented Dec 21, 2023

elBoberido commented Dec 21, 2023

elBoberido left a comment

elBoberido left a comment

elBoberido left a comment

Dennis40816 commented Jan 2, 2024

elBoberido commented Jan 2, 2024

Fix convert edge cases #2135

Fix convert edge cases #2135

Conversation

Dennis40816 commented Dec 15, 2023 • edited by elBoberido Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Dec 15, 2023 • edited Loading

Codecov Report

elBoberido commented Dec 16, 2023

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

Dennis40816 commented Dec 20, 2023

elBoberido commented Dec 20, 2023 • edited Loading

Dennis40816 commented Dec 21, 2023

elBoberido commented Dec 21, 2023

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

Dennis40816 commented Jan 2, 2024

elBoberido commented Jan 2, 2024

Dennis40816 commented Dec 15, 2023 •

edited by elBoberido

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading

elBoberido commented Dec 20, 2023 •

edited

Loading