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

Fix chunkHeader==nullptr in allocateChunk #87

Merged
merged 1 commit into from
May 5, 2020

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Apr 21, 2020

allocateChunk should return nullptr if chunkHeader==nullptr, or it might cause some problem in returning chunkHeader->payload() and user can't check whether allocate chunk successfully or not.

@budrus
Copy link
Contributor

budrus commented Apr 22, 2020

Thanks @evshary . This makes sense.
Internally we switched more to our expected and optional data types in such method return values. We will change the public API with #25 anyway. I'm wondering if we should use this more modern C++ API style also on the top API level. Like here https://github.com/budrus/iceoryx/blob/iox-%2325-chunk-receiver-building-block/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender.hpp#L60

What do you think @evshary @mossmaurice @elBoberido @elfenpiff

@budrus
Copy link
Contributor

budrus commented Apr 22, 2020

@evshary: Can you please change the target branch to staging, as described in https://github.com/eclipse/iceoryx/blob/master/CONTRIBUTING.md

@evshary evshary changed the base branch from master to staging April 22, 2020 10:22
@evshary
Copy link
Contributor Author

evshary commented Apr 22, 2020

@evshary: Can you please change the target branch to staging, as described in https://github.com/eclipse/iceoryx/blob/master/CONTRIBUTING.md

@budrus I guess I make something wrong. Should I rebase this fix to staging branch and then PR?

@budrus
Copy link
Contributor

budrus commented Apr 22, 2020

@evshary: Can you please change the target branch to staging, as described in https://github.com/eclipse/iceoryx/blob/master/CONTRIBUTING.md

@budrus I guess I make something wrong. Should I rebase this fix to staging branch and then PR?

@evshary Sorry, I will rebase staging, Just a second

@budrus
Copy link
Contributor

budrus commented Apr 22, 2020

@evshary: Can you please change the target branch to staging, as described in https://github.com/eclipse/iceoryx/blob/master/CONTRIBUTING.md

@budrus I guess I make something wrong. Should I rebase this fix to staging branch and then PR?

@evshary Sorry, I will rebase staging, Just a second

@evshary . Sorry again, can you switch back to master as target. The staging branch got the same restrictions as master from Eclipse. As I don't have the reviewers now, I cannot update the staging branch. We have to figure out how we can manage the staging branch step in future

@evshary evshary changed the base branch from staging to master April 22, 2020 12:26
@evshary
Copy link
Contributor Author

evshary commented Apr 22, 2020

@evshary: Can you please change the target branch to staging, as described in https://github.com/eclipse/iceoryx/blob/master/CONTRIBUTING.md

@budrus I guess I make something wrong. Should I rebase this fix to staging branch and then PR?

@evshary Sorry, I will rebase staging, Just a second

@evshary . Sorry again, can you switch back to master as target. The staging branch got the same restrictions as master from Eclipse. As I don't have the reviewers now, I cannot update the staging branch. We have to figure out how we can manage the staging branch step in future

Sure.

@budrus budrus merged commit ba6fa88 into eclipse-iceoryx:master May 5, 2020
@elBoberido
Copy link
Member

Thanks @evshary . This makes sense.
Internally we switched more to our expected and optional data types in such method return values. We will change the public API with #25 anyway. I'm wondering if we should use this more modern C++ API style also on the top API level. Like here https://github.com/budrus/iceoryx/blob/iox-%2325-chunk-receiver-building-block/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender.hpp#L60

What do you think @evshary @mossmaurice @elBoberido @elfenpiff

I think we need to decide which type of API we want to have, a type safe C++ API or a lowlevel C-ish API? There a pros and cons for both of them. Personally I wouldn't go the C-ish API since it's to C++ to easily create language bindings and to C to make it type safe.
I think we should probably support a modern C++ API and a C API, one as default and the other one as official binding.

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.

3 participants