You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by arpadboda <gi...@git.apache.org> on 2018/11/15 14:03:01 UTC
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
GitHub user arpadboda opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440
MINIFICPP-676 - Cleanup and fix serializable interface implementation
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 master)?
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arpadboda/nifi-minifi-cpp MINIFICPP-676
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/440.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #440
----
commit cf32a07d7a4b7082a7bc956ea5aa61b343789b81
Author: Arpad Boda <ab...@...>
Date: 2018-11-14T16:06:17Z
MINIFICPP-676 - Cleanup and fix serializable interface implementation
----
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234009476
--- Diff: libminifi/src/io/FileStream.cpp ---
@@ -82,16 +82,16 @@ void FileStream::seek(uint64_t offset) {
file_stream_->seekp(offset_);
}
-int FileStream::writeData(std::vector<uint8_t> &buf, int buflen) {
+int FileStream::writeData(const std::vector<uint8_t> &buf, int buflen) {
if (static_cast<int>(buf.capacity()) < buflen) {
return -1;
}
- return writeData(reinterpret_cast<uint8_t *>(&buf[0]), buflen);
--- End diff --
fails to compile on some of my archs. perhaps run make docker ?
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234006270
--- Diff: extensions/http-curl/client/HTTPStream.h ---
@@ -100,14 +100,14 @@ class HttpStream : public io::BaseStream {
* @param buflen buffer to write
*
*/
- virtual int writeData(std::vector<uint8_t> &buf, int buflen);
+ virtual int writeData(const std::vector<uint8_t> &buf, int buflen);
--- End diff --
Same. C and C++ developers see const and they know what that means. They see the lack of const and know their data may change. If it's non const that doesn't mean it should be changed without discussion. I think these should be non const as we reserve the right to take ownership in the internal API. I would agree for external API.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234007116
--- Diff: libminifi/include/io/CRCStream.h ---
@@ -194,15 +194,12 @@ int CRCStream<T>::readData(uint8_t *buf, int buflen) {
}
template<typename T>
-int CRCStream<T>::writeData(std::vector<uint8_t> &buf, int buflen) {
-
- if ((int)buf.capacity() < buflen)
--- End diff --
This existed for a reason. Did you test this in flight ? This causes a failure. We have non const because we reserve the right to adjust buffers. That's the intent behind making the signatures the way they are.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234008835
--- Diff: libminifi/include/io/Serializable.h ---
@@ -173,8 +147,22 @@ class Serializable {
**/
int readUTF(std::string &str, DataStream *stream, bool widen = false);
- protected:
+ /**
+ * reads 2-8 bytes from the stream
+ * @param value reference in which will set the result
+ * @param stream stream from which we will read
+ * @return resulting read size
+ **/
+ template<typename Integral, typename std::enable_if<
+ (sizeof(Integral) > 1) &&
+ std::is_integral<Integral>::value &&
+ !std::is_signed<Integral>::value
+ ,Integral>::type* = nullptr>
+ int read(Integral &value, DataStream *stream, bool is_little_endian = EndiannessCheck::IS_LITTLE) {
--- End diff --
My IDEs does not make this clear the function to use. In some ways java made this easier with their streams "writeInt, writeShort, writeUnsignedShort" and we can do the same with write(short) write(int), write(long), but my one IDE correctly provides this while others don't. I agree that generally reducing "Needless functions" is a good thing, but here it served a purpose. Happy to further discuss how we can benefit that purpose.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234008240
--- Diff: libminifi/include/io/Serializable.h ---
@@ -22,11 +22,36 @@
#include <string>
#include "EndianCheck.h"
#include "DataStream.h"
+
+namespace {
+ template<typename Integral, typename std::enable_if<
--- End diff --
Far less readable and godbolt and my i7 showed no benefit to this. It seems like syntactical candy. Profiling shows no benefit whatsoever. Having functions with specific functors makes it easy on the development side to specifically type what we set. Your ticket "Type-unsafe template functions" I disagree. I think this is more type unsafe and less easy for developers to read.
"Code duplication" I think this is true, but it results in perhaps some easier development for some. I think we should keep this as is and what was added is syntactical sugar with the potential negative that it can be more confusing during development.
---
[GitHub] nifi-minifi-cpp issue #440: MINIFICPP-676 - Cleanup and fix serializable int...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/440
A step forward, there is improvement left (a lot of code duplication exists in stream hierarchy), although this is enough for one PR.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234011459
--- Diff: extensions/http-curl/client/HTTPCallback.h ---
@@ -78,7 +78,7 @@ class HttpStreamingCallback : public ByteInputCallBack {
}
- virtual int64_t process(uint8_t *vector, size_t size) {
+ virtual int64_t process(const uint8_t *vector, size_t size) {
--- End diff --
in general when i see something like const uint8_t *ptr, then I wonder what the true intent may be there. You have a pointer that can be modified. Should also make that pointer const too, but as with all comments I'm not sure that's the best approach here.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234176325
--- Diff: libminifi/include/io/Serializable.h ---
@@ -22,11 +22,36 @@
#include <string>
#include "EndianCheck.h"
#include "DataStream.h"
+
+namespace {
+ template<typename Integral, typename std::enable_if<
--- End diff --
You are right in the part that this doesn't improve either performance or readibility.
This change became useful in the template write(Integral) function, where this can be called easily, so I don't need to copy-paste writeData calls and could remove the local template writeData implementation.
Overall I see more gain here.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234009236
--- Diff: libminifi/include/io/tls/TLSSocket.h ---
@@ -173,19 +173,19 @@ class TLSSocket : public Socket {
* @param buflen buffer to write
*
*/
- int writeData(std::vector<uint8_t> &buf, int buflen);
+ int writeData(const std::vector<uint8_t> &buf, int buflen);
--- End diff --
if we chose ( and we eventually will in further tickets ) to take this memory and do a memmove ( in a different stream -- can discuss later ) we can't do that with the changes. This was intentional and not an oversight.
---
[GitHub] nifi-minifi-cpp pull request #440: MINIFICPP-676 - Cleanup and fix serializa...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/440#discussion_r234005818
--- Diff: extensions/http-curl/client/HTTPStream.cpp ---
@@ -53,16 +53,16 @@ void HttpStream::seek(uint64_t offset) {
throw std::exception();
}
-int HttpStream::writeData(std::vector<uint8_t> &buf, int buflen) {
+int HttpStream::writeData(const std::vector<uint8_t> &buf, int buflen) {
if ((int)buf.capacity() < buflen) {
return -1;
}
- return writeData(reinterpret_cast<uint8_t *>(&buf[0]), buflen);
--- End diff --
this fails on some compilers. I spent a long time and saw failures on older versions ( not sure if specifically this one ). Did this cause a specific bug? These are non const for a reason and that's to tell the caller that their data may change. You're making an assumption that it shouldn't be const.
---
[GitHub] nifi-minifi-cpp issue #440: MINIFICPP-676 - Cleanup and fix serializable int...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/440
hmmm, I should have checked appveyor because I would have saved some time building on windows...but as I suspected we have issues there too.
---
[GitHub] nifi-minifi-cpp issue #440: MINIFICPP-676 - Cleanup and fix serializable int...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/440
@arpadboda I've run some flows and seen a lot of failures as well.
---