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 2022/01/21 16:12:54 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #1249: MINIFICPP-1669 Use std::byte for buffers
szaszm opened a new pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249
The initial goal was streams only, but it's impossible to change only one part of the code to use std::byte for byte representation.
There is still a bug lurking somewhere, causing SecureSocketGetTCP* tests to fail, but other than that, the PR should be close to final.
---
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?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] 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] szaszm commented on a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800636829
##########
File path: extensions/opencv/FrameIO.h
##########
@@ -57,18 +57,17 @@ class FrameReadCallback : public InputStreamCallback {
~FrameReadCallback() override = default;
int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
- int64_t ret = 0;
- image_buf_.resize(stream->size());
- ret = stream->read(image_buf_.data(), static_cast<int>(stream->size()));
- if (ret < 0 || static_cast<std::size_t>(ret) != stream->size()) {
+ std::vector<uchar> image_buf;
+ image_buf.resize(stream->size());
+ const auto ret = stream->read(gsl::make_span(image_buf).as_span<std::byte>());
+ if (ret != stream->size()) {
throw std::runtime_error("ImageReadCallback failed to fully read flow file input stream");
}
- image_mat_ = cv::imdecode(image_buf_, -1);
- return ret;
+ image_mat_ = cv::imdecode(image_buf, -1);
+ return gsl::narrow<int64_t>(ret);
}
private:
- std::vector<uchar> image_buf_;
cv::Mat &image_mat_;
Review comment:
I looked into it: It stores a reference to an external variable, and the result of `cv::imdecode()` is assigned to that variable, so that looks fine
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r798126300
##########
File path: extensions/standard-processors/processors/DefragmentText.cpp
##########
@@ -187,7 +187,7 @@ struct ReadFlowFileContent : public InputStreamCallback {
int64_t process(const std::shared_ptr<io::BaseStream> &stream) override {
content.resize(stream->size());
- const auto ret = stream->read(reinterpret_cast<uint8_t *>(content.data()), stream->size());
+ const auto ret = stream->read(gsl::make_span(reinterpret_cast<std::byte*>(content.data()), stream->size()));
Review comment:
No particular reason, I just didn't realize at the beginning that there is `as_span` and learned about it later. I'll change them.
--
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 edited a comment on pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm edited a comment on pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#issuecomment-1036293953
I want to make the move in a separate PR.
https://issues.apache.org/jira/browse/MINIFICPP-1755
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r798125860
##########
File path: extensions/opencv/FrameIO.h
##########
@@ -57,18 +57,17 @@ class FrameReadCallback : public InputStreamCallback {
~FrameReadCallback() override = default;
int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
- int64_t ret = 0;
- image_buf_.resize(stream->size());
- ret = stream->read(image_buf_.data(), static_cast<int>(stream->size()));
- if (ret < 0 || static_cast<std::size_t>(ret) != stream->size()) {
+ std::vector<uchar> image_buf;
+ image_buf.resize(stream->size());
+ const auto ret = stream->read(gsl::make_span(image_buf).as_span<std::byte>());
+ if (ret != stream->size()) {
throw std::runtime_error("ImageReadCallback failed to fully read flow file input stream");
}
- image_mat_ = cv::imdecode(image_buf_, -1);
- return ret;
+ image_mat_ = cv::imdecode(image_buf, -1);
+ return gsl::narrow<int64_t>(ret);
}
private:
- std::vector<uchar> image_buf_;
cv::Mat &image_mat_;
Review comment:
I got rid of this file in my other PR, that's why I tried to keep the changes minimal here.
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800652661
##########
File path: libminifi/include/io/AtomicEntryStream.h
##########
@@ -131,23 +132,24 @@ size_t AtomicEntryStream<T>::write(const uint8_t *value, size_t size) {
}
template<typename T>
-size_t AtomicEntryStream<T>::read(uint8_t *buf, size_t buflen) {
- if (buflen == 0) {
+size_t AtomicEntryStream<T>::read(gsl::span<std::byte> buf) {
+ if (buf.empty()) {
return 0;
}
- if (nullptr != buf && !invalid_stream_) {
+ if (!invalid_stream_) {
std::lock_guard<std::recursive_mutex> lock(entry_lock_);
- auto len = buflen;
+ auto len = buf.size();
core::repository::RepoValue<T> *value;
if (entry_->getValue(key_, &value)) {
- if (offset_ + len > value->getBufferSize()) {
- len = gsl::narrow<int>(value->getBufferSize()) - gsl::narrow<int>(offset_);
+ if (offset_ + len > value->getBuffer().size()) {
+ len = value->getBuffer().size() - offset_;
if (len <= 0) {
entry_->decrementOwnership();
return 0;
}
Review comment:
I don't know. This piece of code is way too complex and I find it difficult to reason about its effects, edge cases, etc. I prefer not to touch it any more than the bare minimum required. (this is the 4. nested if block)
I spent a fair amount of time debugging an issue with the change a few lines below this one.
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800669223
##########
File path: libminifi/src/utils/StringUtils.cpp
##########
@@ -268,8 +321,8 @@ bool StringUtils::from_hex(uint8_t* data, size_t* data_length, const char* hex,
return true;
}
-std::vector<uint8_t> StringUtils::from_hex(const char* hex, size_t hex_length) {
- std::vector<uint8_t> decoded(hex_length / 2);
+std::vector<std::byte> StringUtils::from_hex(const char* hex, size_t hex_length) {
+ std::vector<std::byte> decoded(hex_length / 2);
size_t data_length = decoded.size();
if (!from_hex(decoded.data(), &data_length, hex, hex_length)) {
throw std::invalid_argument("Hexencoded string is malformatted");
Review comment:
Chaging both to "malformed". I've never heard/read the word "malformatted" before. After searching the internet for it, it looks like it exists, but is very rarely used, so I'm not convinced that it's not just a common mistake.
--
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 closed pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
martinzink closed pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249
--
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 #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#issuecomment-1036293953
I want to make the move in a separate PR.
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800672706
##########
File path: extensions/standard-processors/processors/DefragmentText.cpp
##########
@@ -187,7 +187,7 @@ struct ReadFlowFileContent : public InputStreamCallback {
int64_t process(const std::shared_ptr<io::BaseStream> &stream) override {
content.resize(stream->size());
- const auto ret = stream->read(reinterpret_cast<uint8_t *>(content.data()), stream->size());
+ const auto ret = stream->read(gsl::make_span(reinterpret_cast<std::byte*>(content.data()), stream->size()));
Review comment:
changed to as_span in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800716297
##########
File path: libminifi/include/utils/StringUtils.h
##########
@@ -423,46 +416,37 @@ class StringUtils {
* @param base64_length the length of base64
* @return true on success
*/
- static bool from_base64(uint8_t* data, size_t* data_length, const char* base64, size_t base64_length);
+ static bool from_base64(std::byte* data, size_t* data_length, std::string_view base64);
/**
* Base64 decodes a string
* @param base64 the Base64 encoded string
* @param base64_length the length of base64
* @return the vector containing the decoded bytes
*/
- static std::vector<uint8_t> from_base64(const char* base64, size_t base64_length);
-
- /**
- * Base64 decodes a string
- * @param base64 the Base64 encoded string
- * @return the decoded string
- */
- inline static std::string from_base64(const std::string& base64) {
- auto data = from_base64(base64.data(), base64.length());
- return std::string(reinterpret_cast<char*>(data.data()), data.size());
+ static std::vector<std::byte> from_base64(std::string_view base64);
+ static std::string from_base64(std::string_view base64, as_string_tag_t) {
Review comment:
I failed to notice this similarity while changing them. Thanks for pointing it out. I made them similar again in [04e00fa](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/04e00fa03c4409b2aabccdf1292698337b70e895).
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800820984
##########
File path: libminifi/src/utils/StringUtils.cpp
##########
@@ -268,8 +321,8 @@ bool StringUtils::from_hex(uint8_t* data, size_t* data_length, const char* hex,
return true;
}
-std::vector<uint8_t> StringUtils::from_hex(const char* hex, size_t hex_length) {
- std::vector<uint8_t> decoded(hex_length / 2);
+std::vector<std::byte> StringUtils::from_hex(const char* hex, size_t hex_length) {
+ std::vector<std::byte> decoded(hex_length / 2);
size_t data_length = decoded.size();
if (!from_hex(decoded.data(), &data_length, hex, hex_length)) {
throw std::invalid_argument("Hexencoded string is malformatted");
Review comment:
It's a thin line between "common mistake" and "it is a word now", but I agree that "malformed" is better.
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800669223
##########
File path: libminifi/src/utils/StringUtils.cpp
##########
@@ -268,8 +321,8 @@ bool StringUtils::from_hex(uint8_t* data, size_t* data_length, const char* hex,
return true;
}
-std::vector<uint8_t> StringUtils::from_hex(const char* hex, size_t hex_length) {
- std::vector<uint8_t> decoded(hex_length / 2);
+std::vector<std::byte> StringUtils::from_hex(const char* hex, size_t hex_length) {
+ std::vector<std::byte> decoded(hex_length / 2);
size_t data_length = decoded.size();
if (!from_hex(decoded.data(), &data_length, hex, hex_length)) {
throw std::invalid_argument("Hexencoded string is malformatted");
Review comment:
Chaging both to "malformed". I've never heard/read the word "malformatted" before. After searching the internet for it, it looks like it exists, but is very rarely used, so I'm not convinced that it's not just a common mistake.
[30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -141,17 +141,15 @@ size_t FileStream::write(const uint8_t *value, size_t size) {
return size;
}
-size_t FileStream::read(uint8_t *buf, size_t buflen) {
- if (buflen == 0) {
- return 0;
- }
+size_t FileStream::read(gsl::span<std::byte> buf) {
+ if (buf.empty()) { return 0; }
if (!IsNullOrEmpty(buf)) {
Review comment:
done in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
##########
File path: libminifi/src/c2/C2Payload.cpp
##########
@@ -64,20 +64,20 @@ void C2Payload::addContent(C2ContentResponse &&content, bool collapsible) {
}
void C2Payload::setRawData(const std::string &data) {
+ const auto byte_view = gsl::make_span(data).as_span<const std::byte>();
raw_data_.reserve(raw_data_.size() + data.size());
- raw_data_.insert(std::end(raw_data_), std::begin(data), std::end(data));
+ raw_data_.insert(std::end(raw_data_), std::begin(byte_view), std::end(byte_view));
}
void C2Payload::setRawData(const std::vector<char> &data) {
+ const auto byte_view = gsl::make_span(data).as_span<const std::byte>();
raw_data_.reserve(raw_data_.size() + data.size());
- raw_data_.insert(std::end(raw_data_), std::begin(data), std::end(data));
+ raw_data_.insert(std::end(raw_data_), std::begin(byte_view), std::end(byte_view));
}
Review comment:
done in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800672928
##########
File path: controller/Controller.h
##########
@@ -111,12 +111,12 @@ int getFullConnections(std::unique_ptr<org::apache::nifi::minifi::io::Socket> so
org::apache::nifi::minifi::io::BufferStream stream;
stream.write(&op, 1);
stream.write("getfull");
- if (org::apache::nifi::minifi::io::isError(socket->write(stream.getBuffer(), stream.size()))) {
+ if (org::apache::nifi::minifi::io::isError(socket->write(stream.getBuffer()))) {
return -1;
}
// read the response
uint8_t resp = 0;
- socket->read(&resp, 1);
+ socket->read(gsl::make_span(reinterpret_cast<std::byte*>(&resp), 1));
Review comment:
yes, done in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800672565
##########
File path: libminifi/include/c2/PayloadSerializer.h
##########
@@ -209,7 +209,7 @@ class PayloadSerializer {
}
return node;
}
- static C2Payload deserialize(std::vector<uint8_t> data) {
+ static C2Payload deserialize(std::vector<std::byte> data) {
Review comment:
changed to const& in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800672312
##########
File path: libminifi/include/io/ClientSocket.h
##########
@@ -137,8 +137,8 @@ class Socket : public BaseStream {
* @param buflen
* @param retrieve_all_bytes determines if we should read all bytes before returning
*/
- size_t read(uint8_t *buf, size_t buflen) override {
- return read(buf, buflen, true);
+ size_t read(gsl::span<std::byte> buf) override {
+ return read(buf, false);
Review comment:
Looks like a mistake, changed back to true in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r797434591
##########
File path: extensions/opencv/FrameIO.h
##########
@@ -57,18 +57,17 @@ class FrameReadCallback : public InputStreamCallback {
~FrameReadCallback() override = default;
int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
- int64_t ret = 0;
- image_buf_.resize(stream->size());
- ret = stream->read(image_buf_.data(), static_cast<int>(stream->size()));
- if (ret < 0 || static_cast<std::size_t>(ret) != stream->size()) {
+ std::vector<uchar> image_buf;
+ image_buf.resize(stream->size());
+ const auto ret = stream->read(gsl::make_span(image_buf).as_span<std::byte>());
+ if (ret != stream->size()) {
throw std::runtime_error("ImageReadCallback failed to fully read flow file input stream");
}
- image_mat_ = cv::imdecode(image_buf_, -1);
- return ret;
+ image_mat_ = cv::imdecode(image_buf, -1);
+ return gsl::narrow<int64_t>(ret);
}
private:
- std::vector<uchar> image_buf_;
cv::Mat &image_mat_;
Review comment:
This is old code, but it looks very broken: we store a reference to the temporary returned by `cv::imdecode()`.
##########
File path: libminifi/include/io/AtomicEntryStream.h
##########
@@ -131,23 +132,24 @@ size_t AtomicEntryStream<T>::write(const uint8_t *value, size_t size) {
}
template<typename T>
-size_t AtomicEntryStream<T>::read(uint8_t *buf, size_t buflen) {
- if (buflen == 0) {
+size_t AtomicEntryStream<T>::read(gsl::span<std::byte> buf) {
+ if (buf.empty()) {
return 0;
}
- if (nullptr != buf && !invalid_stream_) {
+ if (!invalid_stream_) {
std::lock_guard<std::recursive_mutex> lock(entry_lock_);
- auto len = buflen;
+ auto len = buf.size();
core::repository::RepoValue<T> *value;
if (entry_->getValue(key_, &value)) {
- if (offset_ + len > value->getBufferSize()) {
- len = gsl::narrow<int>(value->getBufferSize()) - gsl::narrow<int>(offset_);
+ if (offset_ + len > value->getBuffer().size()) {
+ len = value->getBuffer().size() - offset_;
if (len <= 0) {
entry_->decrementOwnership();
return 0;
}
Review comment:
Is this dead code? Or should the `>` in line 144 be a `>=`?
##########
File path: libminifi/src/c2/C2Payload.cpp
##########
@@ -64,20 +64,20 @@ void C2Payload::addContent(C2ContentResponse &&content, bool collapsible) {
}
void C2Payload::setRawData(const std::string &data) {
+ const auto byte_view = gsl::make_span(data).as_span<const std::byte>();
raw_data_.reserve(raw_data_.size() + data.size());
- raw_data_.insert(std::end(raw_data_), std::begin(data), std::end(data));
+ raw_data_.insert(std::end(raw_data_), std::begin(byte_view), std::end(byte_view));
}
void C2Payload::setRawData(const std::vector<char> &data) {
+ const auto byte_view = gsl::make_span(data).as_span<const std::byte>();
raw_data_.reserve(raw_data_.size() + data.size());
- raw_data_.insert(std::end(raw_data_), std::begin(data), std::end(data));
+ raw_data_.insert(std::end(raw_data_), std::begin(byte_view), std::end(byte_view));
}
Review comment:
These two could call `setRawData(gsl::span)` instead of duplicating it.
##########
File path: libminifi/include/utils/StringUtils.h
##########
@@ -423,46 +416,37 @@ class StringUtils {
* @param base64_length the length of base64
* @return true on success
*/
- static bool from_base64(uint8_t* data, size_t* data_length, const char* base64, size_t base64_length);
+ static bool from_base64(std::byte* data, size_t* data_length, std::string_view base64);
/**
* Base64 decodes a string
* @param base64 the Base64 encoded string
* @param base64_length the length of base64
* @return the vector containing the decoded bytes
*/
- static std::vector<uint8_t> from_base64(const char* base64, size_t base64_length);
-
- /**
- * Base64 decodes a string
- * @param base64 the Base64 encoded string
- * @return the decoded string
- */
- inline static std::string from_base64(const std::string& base64) {
- auto data = from_base64(base64.data(), base64.length());
- return std::string(reinterpret_cast<char*>(data.data()), data.size());
+ static std::vector<std::byte> from_base64(std::string_view base64);
+ static std::string from_base64(std::string_view base64, as_string_tag_t) {
Review comment:
`from_hex` and `from_base64` used to have the same set of overloads, and `to_hex` and `to_base64` as well, if we ignore the bool flags. Now the hex and base64 functions are completely different.
I am fine with either version, but I would prefer if hex and base64 were the same, as before.
##########
File path: libminifi/include/utils/ByteArrayCallback.h
##########
@@ -46,7 +42,7 @@ class ByteInputCallBack : public InputStreamCallback {
if (stream->size() > 0) {
vec.resize(stream->size());
- stream->read(reinterpret_cast<uint8_t*>(vec.data()), stream->size());
+ stream->read(gsl::make_span(reinterpret_cast<std::byte*>(vec.data()), vec.size()));
Review comment:
`vec` is already a `vector<byte>`, so could we write `stream->read(vec);`?
##########
File path: libminifi/include/io/ClientSocket.h
##########
@@ -137,8 +137,8 @@ class Socket : public BaseStream {
* @param buflen
* @param retrieve_all_bytes determines if we should read all bytes before returning
*/
- size_t read(uint8_t *buf, size_t buflen) override {
- return read(buf, buflen, true);
+ size_t read(gsl::span<std::byte> buf) override {
+ return read(buf, false);
Review comment:
why did this change from `true` to `false`?
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -141,17 +141,15 @@ size_t FileStream::write(const uint8_t *value, size_t size) {
return size;
}
-size_t FileStream::read(uint8_t *buf, size_t buflen) {
- if (buflen == 0) {
- return 0;
- }
+size_t FileStream::read(gsl::span<std::byte> buf) {
+ if (buf.empty()) { return 0; }
if (!IsNullOrEmpty(buf)) {
Review comment:
this `IsNulOrEmpty` check could be removed, as you did in DescriptorStream.cpp
##########
File path: libminifi/include/c2/PayloadSerializer.h
##########
@@ -209,7 +209,7 @@ class PayloadSerializer {
}
return node;
}
- static C2Payload deserialize(std::vector<uint8_t> data) {
+ static C2Payload deserialize(std::vector<std::byte> data) {
Review comment:
I guess it's not in scope for this change, but all this copying of the `data` vector seems unnecessary.
##########
File path: controller/Controller.h
##########
@@ -111,12 +111,12 @@ int getFullConnections(std::unique_ptr<org::apache::nifi::minifi::io::Socket> so
org::apache::nifi::minifi::io::BufferStream stream;
stream.write(&op, 1);
stream.write("getfull");
- if (org::apache::nifi::minifi::io::isError(socket->write(stream.getBuffer(), stream.size()))) {
+ if (org::apache::nifi::minifi::io::isError(socket->write(stream.getBuffer()))) {
return -1;
}
// read the response
uint8_t resp = 0;
- socket->read(&resp, 1);
+ socket->read(gsl::make_span(reinterpret_cast<std::byte*>(&resp), 1));
Review comment:
Could we use the `read(Integral& value)` overload here (and at lines 145, 183) as you did at line 89? That would look nicer.
##########
File path: libminifi/src/utils/StringUtils.cpp
##########
@@ -268,8 +321,8 @@ bool StringUtils::from_hex(uint8_t* data, size_t* data_length, const char* hex,
return true;
}
-std::vector<uint8_t> StringUtils::from_hex(const char* hex, size_t hex_length) {
- std::vector<uint8_t> decoded(hex_length / 2);
+std::vector<std::byte> StringUtils::from_hex(const char* hex, size_t hex_length) {
+ std::vector<std::byte> decoded(hex_length / 2);
size_t data_length = decoded.size();
if (!from_hex(decoded.data(), &data_length, hex, hex_length)) {
throw std::invalid_argument("Hexencoded string is malformatted");
Review comment:
I don't mind either way, but `from_hex` and `from_base64` should either both use "malformed" or both use "malformatted".
##########
File path: extensions/standard-processors/processors/DefragmentText.cpp
##########
@@ -187,7 +187,7 @@ struct ReadFlowFileContent : public InputStreamCallback {
int64_t process(const std::shared_ptr<io::BaseStream> &stream) override {
content.resize(stream->size());
- const auto ret = stream->read(reinterpret_cast<uint8_t *>(content.data()), stream->size());
+ const auto ret = stream->read(gsl::make_span(reinterpret_cast<std::byte*>(content.data()), stream->size()));
Review comment:
This is fine, too, but why not `stream->read(gsl::make_span(content).as_span<std::byte>())`, as you did elsewhere?
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r798126742
##########
File path: libminifi/include/c2/PayloadSerializer.h
##########
@@ -209,7 +209,7 @@ class PayloadSerializer {
}
return node;
}
- static C2Payload deserialize(std::vector<uint8_t> data) {
+ static C2Payload deserialize(std::vector<std::byte> data) {
Review comment:
I don't mind small extra changes like this, I only avoid the large scope expansions. Will change.
--
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 a change in pull request #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#discussion_r800672107
##########
File path: libminifi/include/utils/ByteArrayCallback.h
##########
@@ -46,7 +42,7 @@ class ByteInputCallBack : public InputStreamCallback {
if (stream->size() > 0) {
vec.resize(stream->size());
- stream->read(reinterpret_cast<uint8_t*>(vec.data()), stream->size());
+ stream->read(gsl::make_span(reinterpret_cast<std::byte*>(vec.data()), vec.size()));
Review comment:
done in [30de0dc](https://github.com/apache/nifi-minifi-cpp/pull/1249/commits/30de0dc2abaed028ebf8ebb08be2bd9fd86edb52)
--
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 #1249: MINIFICPP-1669 Use std::byte for buffers
Posted by GitBox <gi...@apache.org>.
martinzink commented on pull request #1249:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1249#issuecomment-1036278056
I was just checking the c++20 features and we should be able to use std::span, any reason you are using gsl::span instead of std::span?
Maybe we can move to std::span in a separate PR?
--
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