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.

Sign up for GitHub

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

Jump to bottom

Iox #408 rename publisher and subscriber #561

Merged
merged 9 commits into from
Feb 16, 2021
Merged

Iox #408 rename publisher and subscriber #561

merged 9 commits into from
Feb 16, 2021

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Feb 16, 2021 •
edited
Loading

Pre-Review Checklist for the PR Author

  1. Branch follows the naming format (iox-#123-this-is-a-branch)
  2. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  3. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  4. Relevant issues are linked
  5. Add sensible notes for the reviewer
  6. All checks have passed (except task-list-completed)
  7. Assign PR to reviewer

Notes for Reviewer

This PR renames the the TypedPublisher and TypedSubscriber to just Publisher and Subscriber as decided in #408. Renaming the untyped API will be done in a separate PR if it is still desired.

The typed_publisher.hpp and typed_subscirber.hpp were renamed with git mv but since there was already a publisher.hpp and subscriber.hpp git assumes those old files where changed.

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
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

  1. All checkboxes in the PR checklist are checked or crossed out
  2. Merge

References

  • Partly closes 408

@elBoberido
iox-eclipse-iceoryx#408 Remove subscriber.hpp which just included the…
25d8cec 
… typed and untyped variant

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Rename typed_subscriber files to just `subs…
60c4e37 
…criber`

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Rename TypedSubscriber to just Subscriber
785e44c 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Remove publisher.hpp which just included the …
96afc4e 
…typed and untyped variant

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Rename typed_publisher files to just `publi…
02001af 
…sher`

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Rename TypedPublisher to just Publisher
6523894 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido elBoberido added the refactoring Refactor code without adding features label Feb 16, 2021
@elBoberido elBoberido self-assigned this Feb 16, 2021
@elBoberido
iox-eclipse-iceoryx#408 Update PR template
a46273d 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#408 Fix address sanitizer issue
442de34 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido elBoberido force-pushed the iox-#408-rename-publisher-and-subscriber branch from 2b8c192 to c68ebbb Compare February 16, 2021 13:53
dkroenke
dkroenke reviewed Feb 16, 2021
View reviewed changes
iceoryx_posh/test/moduletests/test_popo_base_subscriber.cpp
// anonymous namespace to prevent linker issues or sanitizer false positives
// if a struct with the same name is used in other tests
namespace
{
struct DummyData
{
uint64_t val = 42;
Copy link
Member

Choose a reason for hiding this comment

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

👀

elfenpiff
elfenpiff previously approved these changes Feb 16, 2021
View reviewed changes
iceoryx_posh/test/moduletests/test_popo_sample.cpp
struct DummyData
{
DummyData() = default;
int val = 42;
uint32_t val = 42;
Copy link
Contributor

Choose a reason for hiding this comment

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

🐙 <- @dkroenke

Copy link
Member Author

Choose a reason for hiding this comment

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

btw. that was the reason for the address sanitizer issue. There were 4 structs with the same name, three with uint64_t and one with int all in the same binary. Somehow the address sanitizer mixed them up.

@elBoberido
iox-eclipse-iceoryx#408 Update copyright
5ae8fd4 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido elBoberido force-pushed the iox-#408-rename-publisher-and-subscriber branch from c68ebbb to 5ae8fd4 Compare February 16, 2021 16:45
MatthiasKillat
MatthiasKillat approved these changes Feb 16, 2021
View reviewed changes
iceoryx_posh/test/moduletests/test_popo_base_subscriber.cpp
@@ -31,10 +31,15 @@
using namespace ::testing;
using ::testing::_;

// anonymous namespace to prevent linker issues or sanitizer false positives
Copy link
Contributor

Choose a reason for hiding this comment

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

I was aware of this and always have all my tests in a local anonymous namespace. This should be in the test recommendation file as well, if if is not already mentioned there.

Avoid weird linker issues, some of them seemingly silent...

iceoryx_posh/test/moduletests/test_popo_untyped_subscriber.cpp
struct DummyData
{
uint64_t val = 42;
};
} // namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

I would extend this to the test fixture or even the tests themselves. Test fixture names can also cause conflicts.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in #563

@mossmaurice mossmaurice linked an issue Feb 16, 2021 that may be closed by this pull request
Refine modern pub/sub API #408
Closed
5 tasks
@MatthiasKillat
Copy link
Contributor

I would recommend adding or extending anonymous namespaces to all tests in a separate PR to avoid future linker/address sanitizer issues. The should cover all definitions in the corresponding test_xyz.cpp file.

@elBoberido elBoberido mentioned this pull request Feb 16, 2021
Closed
6 tasks
@elBoberido elBoberido merged commit 9812e3d into eclipse-iceoryx:master Feb 16, 2021
@elBoberido elBoberido deleted the iox-#408-rename-publisher-and-subscriber branch February 16, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossmaurice mossmaurice mossmaurice approved these changes

@MatthiasKillat MatthiasKillat MatthiasKillat approved these changes

@dkroenke dkroenke Awaiting requested review from dkroenke

@elfenpiff elfenpiff Awaiting requested review from elfenpiff

Assignees

@elBoberido elBoberido

Labels
refactoring Refactor code without adding features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refine modern pub/sub API
5 participants
@elBoberido @MatthiasKillat @mossmaurice @dkroenke @elfenpiff