Skip to content

Store dedicated transform listener thread as a std::unique_ptr#111

Merged
allenh1 merged 3 commits intoros2from
store-transform-listener-thread-in-unique-ptr
May 22, 2019
Merged

Store dedicated transform listener thread as a std::unique_ptr#111
allenh1 merged 3 commits intoros2from
store-transform-listener-thread-in-unique-ptr

Conversation

@allenh1
Copy link
Contributor

@allenh1 allenh1 commented May 11, 2019

Signed-off-by: Hunter L. Allen hunterlallen@protonmail.com

@allenh1 allenh1 added enhancement New feature or request in review Waiting for review (Kanban column) labels May 11, 2019
@allenh1 allenh1 requested a review from Karsten1987 May 11, 2019 18:59
@allenh1 allenh1 self-assigned this May 11, 2019
@tfoote
Copy link
Contributor

tfoote commented May 13, 2019

Follow up to #108 (comment)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I don't see the big benefit of converting the raw pointer to a unique_ptr if not putting the rest of the destructor logic in a custom deleter and thus having the unique ptr taking care of the dedicated thread itself.

@Karsten1987
Copy link
Contributor

@allenh1 is there any update from your side? Would be great to have this PR landed before dashing.

@allenh1
Copy link
Contributor Author

allenh1 commented May 21, 2019

@Karsten1987 my bad, I'll get to this ASAP.

@allenh1 allenh1 force-pushed the store-transform-listener-thread-in-unique-ptr branch from fd41a8c to b813c49 Compare May 21, 2019 20:24
@allenh1
Copy link
Contributor Author

allenh1 commented May 21, 2019

@Karsten1987 ptal -- this is kind of clean, but not my favorite patch. I couldn't get the std::unique_ptr to cooperate without declaring it with two template args (since it will default initialize it in other ways). I ended up with a using clause; let me know if you want me to kill it and, if so, I'll just inline the definitions.

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

Can we delete using_dedicated_thread_ then? It looks pretty unnecessary to me. With it, we can also delete the private method dedicatedListenerThread which doesn't do anything.
This should IMO not break API but max ABI, which at this point should still be ok for dashing.

@tfoote opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

my personal style would just be to put the using just above the first (and only) declaration of this type in line 115.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's only used once that is a lot easier to read.

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Yes, we can clear the old flag using_dedicated_thread_ and the old unused function dedicatedListenerThread that's replaced by the lambda. We should move the TODO from that method up to the lambda to remind us to come back and implement it with callback groups when callbackgroups become available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's only used once that is a lot easier to read.

allenh1 added 3 commits May 22, 2019 10:24
Signed-off-by: Hunter L. Allen <hunterlallen@protonmail.com>
…nullptr check may be removed

Signed-off-by: Hunter L. Allen <hunterlallen@protonmail.com>
Signed-off-by: Hunter L. Allen <hunterlallen@protonmail.com>
@allenh1 allenh1 force-pushed the store-transform-listener-thread-in-unique-ptr branch from 7574599 to 8f82d0d Compare May 22, 2019 14:26
@allenh1 allenh1 requested a review from tfoote May 22, 2019 14:26
@allenh1
Copy link
Contributor Author

allenh1 commented May 22, 2019

@Karsten1987 / @tfoote Could you please spin up the CI for me?

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

lgtm, one last ci run:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@allenh1
Copy link
Contributor Author

allenh1 commented May 22, 2019

Woot

@allenh1 allenh1 merged commit 33f90e4 into ros2 May 22, 2019
@allenh1 allenh1 deleted the store-transform-listener-thread-in-unique-ptr branch May 22, 2019 19:24
@allenh1 allenh1 removed the in review Waiting for review (Kanban column) label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants