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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
