You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/09/29 14:29:34 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
szaszm opened a new pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182
Done with a limited number of extensions until I can gather feedback. I propose that we get rid of the *StreamCallback base classes and just use std::function to type erase the callback types where necessary. The reason is simlicity of implementation and usage.
The code on the PR branch is tested with these cmake flags: `-DFORCE_COLORED_OUTPUT=ON -DAWS_ENABLE_UNITY_BUILD=OFF -DASAN_BUILD=OFF -DENABLE_PYTHON=ON -DENABLE_OPS=ON -DENABLE_JNI=OFF -DENABLE_OPC=OFF -DENABLE_COAP=ON -DENABLE_GPS=ON -DENABLE_MQTT=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_SENSORS=OFF -DENABLE_USB_CAMERA=ON -DENABLE_AWS=ON -DENABLE_SFTP=ON -DENABLE_OPENWSMAN=OFF -DENABLE_BUSTACHE=OFF -DENABLE_OPENCV=OFF -DENABLE_TENSORFLOW=OFF -DENABLE_SQL=ON -DENABLE_PCAP=OFF -DENABLE_LUA_SCRIPTING=ON -DENABLE_NANOFI=ON -DENABLE_AZURE=ON`
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
martinzink commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-947571811
I like this initiative :+1:
Wanna mention that https://issues.apache.org/jira/browse/MINIFICPP-1648 (currently unassigned) and this would cause a lot of conflicts if worked on parallelly.
If you move forward with this please link these issues together so we don't create unnecessary work for ourselves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-945746873
1. If you manage state with lambda captures, then it works file. `std::function` only supports value semantics, so even reference forwarding wouldn't work, unless we converted the functions to templates on the actual callback type instead of type erasure, or reimplemented `std::function` with reference semantics.
2. Ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
fgerlits commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-945756522
this would work: https://godbolt.org/z/zs7v9bdv6, but yeah, it's not ideal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
fgerlits commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-945741723
I like this. Just two comments for discussion:
1. using a `const&` {Input,Output}StreamCallback parameter, which makes the `std::ref()` necessary, does not feel natural to me. If I was to write new code with a stateful callback, I would not know I'd need `std::ref()`, and then spend a fair bit of time trying to find out why my code doesn't work. I would prefer a universal reference parameter, but I don't like having to put `read()` etc in the header. Maybe someone has a better idea?
2. [[predictable]] I would put the stream `shared_ptr` to `&` change in a separate pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-947573203
I'll link them, but there is no issue for this yet. This is mostly here just to gather feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-945765651
I think keeping `std::function`, callback value semantics, and using `std::ref` where necessary is better design. The relatively common usage is only a result of converting old code without rewriting the callbacks, but I think the new structure encourages stateless callback design naturally, so it wouldn't be as commonly used as it is now.
Here it's not an issue, but generally there is a problem with forwarding function objects: If they are rvalues, then we can only call them once, since we can't safely reuse a `move`d-from parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1182: DRAFT: Refactor *StreamCallback to be function objects
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1182:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1182#issuecomment-1005701626
Closing this for now, will reopen when I have something final-ish ready
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org