You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "adamdebreceni (via GitHub)" <gi...@apache.org> on 2023/06/06 13:17:45 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request, #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
adamdebreceni opened a new pull request, #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1229501148
##########
libminifi/test/TestBase.cpp:
##########
@@ -645,7 +645,7 @@ std::string TestPlan::getContent(const minifi::core::FlowFile& file) const {
auto content_stream = content_repo_->read(*content_claim);
auto output_stream = std::make_shared<minifi::io::BufferStream>();
minifi::InputStreamPipe{*output_stream}(content_stream);
- return utils::span_to<std::string>(output_stream->getBuffer().as_span<const char>());
+ return std::string{reinterpret_cast<const char*>(output_stream->getBuffer().data()), output_stream->size()};
Review Comment:
done
--
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1229507358
##########
libminifi/test/rocksdb-tests/SwapTests.cpp:
##########
@@ -57,7 +57,7 @@ class OutputProcessor : public core::Processor {
auto ff = session->create();
ff->addAttribute("index", id);
session->write(ff, [&] (const std::shared_ptr<minifi::io::OutputStream>& output) -> int64_t {
- auto ret = output->write(gsl::span<const char>(id.data(), id.size()).as_span<const std::byte>());
+ auto ret = output->write(as_bytes(std::span<const char>(id.data(), id.size())));
Review Comment:
directly calling `as_bytes` on an `std::string` fails, but added `std::span` ctor
--
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 diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1229570316
##########
libminifi/include/utils/gsl.h:
##########
@@ -17,6 +17,7 @@
#pragma once
+#include <span>
Review Comment:
Now that the utils were moved, do we need this include?
##########
libminifi/include/utils/span.h:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <span>
+
+namespace org::apache::nifi::minifi::utils {
+
+namespace detail {
+template<typename T>
+using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>::type>::type;
+} // namespace detail
+
+template<typename Container, typename T>
+Container span_to(std::span<T> span) {
+ static_assert(std::is_constructible<Container, typename std::span<T>::iterator, typename std::span<T>::iterator>::value,
+ "The destination container must have an iterator (pointer) range constructor");
+ return Container(std::begin(span), std::end(span));
+}
+template<template<typename...> class Container, typename T>
+Container<detail::remove_cvref_t<T>> span_to(std::span<T> span) {
+ static_assert(std::is_constructible<Container<detail::remove_cvref_t<T>>, typename std::span<T>::iterator, typename std::span<T>::iterator>::value,
+ "The destination container must have an iterator (pointer) range constructor");
+ return span_to<Container<detail::remove_cvref_t<T>>>(span);
+}
+
+template<typename T, typename U>
+std::span<T> as_span(std::span<U> value) {
+ return std::span{reinterpret_cast<T*>(value.data()), value.size_bytes() / sizeof(T)};
+}
Review Comment:
I believe it's undefined behavior to access the result of this unless it's the same (without cv-qual.) as the original contained type, `unsigned char` or `std::byte` (for which we use `std::as_bytes`/`std::as_writable_bytes`), or another trivial layout-compatible type.
I don't know a better solution for now: `memcpy`/`std::bit_cast` everywhere seems a bit excessive, will cause ownership problems, and the current non-standard approach still works, and will most likely keep working with at least GCC. (They generally don't like to break such things, even if non-standard.)
Can we add a comment about this? Feel free to simplify the language or omit some details, as you see fit.
--
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1251928593
##########
libminifi/src/utils/LineByLineInputOutputStreamCallback.cpp:
##########
@@ -67,7 +68,7 @@ void LineByLineInputOutputStreamCallback::readLine() {
if (end_of_line != input_.end()) { ++end_of_line; }
current_line_ = next_line_;
- next_line_ = utils::span_to<std::string>(gsl::make_span(&*current_pos_, &*end_of_line).as_span<char>());
+ next_line_ = utils::span_to<std::string>(utils::as_span<char>(std::span(std::to_address(current_pos_), std::to_address(end_of_line))));
Review Comment:
according to the requirements of [contiguous_iterator](https://en.cppreference.com/w/cpp/iterator/contiguous_iterator) it should work
--
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] lordgamez commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1251745089
##########
libminifi/include/core/repository/AtomicRepoEntries.h:
##########
@@ -18,6 +18,7 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_ATOMICREPOENTRIES_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_ATOMICREPOENTRIES_H_
Review Comment:
Could replace this with `#pragma once`
--
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 diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1221440952
##########
libminifi/src/utils/LineByLineInputOutputStreamCallback.cpp:
##########
@@ -67,7 +67,7 @@ void LineByLineInputOutputStreamCallback::readLine() {
if (end_of_line != input_.end()) { ++end_of_line; }
current_line_ = next_line_;
- next_line_ = utils::span_to<std::string>(gsl::make_span(&*current_pos_, &*end_of_line).as_span<char>());
+ next_line_ = utils::span_to<std::string>(utils::as_span<char>(std::span(&*current_pos_, &*end_of_line)));
Review Comment:
I think we could remove the `&*`s:
```suggestion
next_line_ = utils::span_to<std::string>(utils::as_span<char>(std::span(current_pos_, end_of_line)));
```
as there is a `span` constructor which takes a pair of iterators
##########
libminifi/include/utils/gsl.h:
##########
@@ -32,18 +33,23 @@ using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>
} // namespace detail
template<typename Container, typename T>
-Container span_to(gsl::span<T> span) {
+Container span_to(std::span<T> span) {
static_assert(std::is_constructible<Container, typename gsl::span<T>::iterator, typename gsl::span<T>::iterator>::value,
"The destination container must have an iterator (pointer) range constructor");
return Container(std::begin(span), std::end(span));
}
template<template<typename...> class Container, typename T>
-Container<detail::remove_cvref_t<T>> span_to(gsl::span<T> span) {
+Container<detail::remove_cvref_t<T>> span_to(std::span<T> span) {
static_assert(std::is_constructible<Container<detail::remove_cvref_t<T>>, typename gsl::span<T>::iterator, typename gsl::span<T>::iterator>::value,
"The destination container must have an iterator (pointer) range constructor");
return span_to<Container<detail::remove_cvref_t<T>>>(span);
}
+template<typename T, typename U>
+std::span<T> as_span(std::span<U> value) {
+ return std::span{reinterpret_cast<T*>(value.data()), value.size_bytes() / sizeof(T)};
+}
Review Comment:
the `span_to` and `as_span` functions could be moved to a new file, eg. `utils/span.h`, as they don't belong in `gsl.h` any longer
--
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1229792431
##########
libminifi/include/utils/gsl.h:
##########
@@ -17,6 +17,7 @@
#pragma once
+#include <span>
Review Comment:
removed
--
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 diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1222182466
##########
libminifi/include/utils/crypto/ciphers/Aes256Ecb.h:
##########
@@ -50,8 +50,8 @@ class Aes256EcbCipher {
static constexpr size_t KEY_SIZE = 32;
explicit Aes256EcbCipher(Bytes encryption_key);
- void encrypt(gsl::span<unsigned char /*, BLOCK_SIZE*/> data) const;
- void decrypt(gsl::span<unsigned char /*, BLOCK_SIZE*/> data) const;
+ void encrypt(std::span<unsigned char /*, BLOCK_SIZE*/> data) const;
+ void decrypt(std::span<unsigned char /*, BLOCK_SIZE*/> data) const;
Review Comment:
Maybe we could change to a fixed extent span here?
##########
libminifi/test/TestBase.cpp:
##########
@@ -645,7 +645,7 @@ std::string TestPlan::getContent(const minifi::core::FlowFile& file) const {
auto content_stream = content_repo_->read(*content_claim);
auto output_stream = std::make_shared<minifi::io::BufferStream>();
minifi::InputStreamPipe{*output_stream}(content_stream);
- return utils::span_to<std::string>(output_stream->getBuffer().as_span<const char>());
+ return std::string{reinterpret_cast<const char*>(output_stream->getBuffer().data()), output_stream->size()};
Review Comment:
I'd prefer to keep following the as_span<const char> -> span_to<string> pattern here as well.
##########
libminifi/test/unit/GslTest.cpp:
##########
@@ -1,35 +0,0 @@
-/**
- *
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <vector>
-#include <string>
-#include "../TestBase.h"
-#include "../Catch.h"
-#include "utils/gsl.h"
-
-namespace utils = org::apache::nifi::minifi::utils;
-
-TEST_CASE("span to", "[span to]") {
- const auto test_span = gsl::make_span("test text", 9);
- const auto string = utils::span_to<std::string>(test_span);
- const auto vector = utils::span_to<std::vector>(test_span);
-
- REQUIRE(string == "test text");
- REQUIRE('t' == vector[0]);
- REQUIRE(9 == vector.size());
-}
Review Comment:
why is this deleted? we should also add tests for as_span.
##########
libminifi/test/rocksdb-tests/SwapTests.cpp:
##########
@@ -57,7 +57,7 @@ class OutputProcessor : public core::Processor {
auto ff = session->create();
ff->addAttribute("index", id);
session->write(ff, [&] (const std::shared_ptr<minifi::io::OutputStream>& output) -> int64_t {
- auto ret = output->write(gsl::span<const char>(id.data(), id.size()).as_span<const std::byte>());
+ auto ret = output->write(as_bytes(std::span<const char>(id.data(), id.size())));
Review Comment:
Could we skip the explicit `std::span` construction, and just rely on implicit conversion from `string` to `span` in the argument of `as_bytes`?
```suggestion
auto ret = output->write(as_bytes(id));
```
##########
libminifi/include/utils/gsl.h:
##########
@@ -32,18 +33,23 @@ using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>
} // namespace detail
template<typename Container, typename T>
-Container span_to(gsl::span<T> span) {
+Container span_to(std::span<T> span) {
Review Comment:
Please check the definition as well. There are still references to `gsl::span` in the assertions.
--
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1229795382
##########
libminifi/src/utils/LineByLineInputOutputStreamCallback.cpp:
##########
@@ -67,7 +67,7 @@ void LineByLineInputOutputStreamCallback::readLine() {
if (end_of_line != input_.end()) { ++end_of_line; }
current_line_ = next_line_;
- next_line_ = utils::span_to<std::string>(gsl::make_span(&*current_pos_, &*end_of_line).as_span<char>());
+ next_line_ = utils::span_to<std::string>(utils::as_span<char>(std::span(&*current_pos_, &*end_of_line)));
Review Comment:
I had to revert this as there doesn't seems to be an iterator overload constructor in the macos ci runner
##########
libminifi/include/utils/span.h:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <span>
+
+namespace org::apache::nifi::minifi::utils {
+
+namespace detail {
+template<typename T>
+using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>::type>::type;
+} // namespace detail
+
+template<typename Container, typename T>
+Container span_to(std::span<T> span) {
+ static_assert(std::is_constructible<Container, typename std::span<T>::iterator, typename std::span<T>::iterator>::value,
+ "The destination container must have an iterator (pointer) range constructor");
+ return Container(std::begin(span), std::end(span));
+}
+template<template<typename...> class Container, typename T>
+Container<detail::remove_cvref_t<T>> span_to(std::span<T> span) {
+ static_assert(std::is_constructible<Container<detail::remove_cvref_t<T>>, typename std::span<T>::iterator, typename std::span<T>::iterator>::value,
+ "The destination container must have an iterator (pointer) range constructor");
+ return span_to<Container<detail::remove_cvref_t<T>>>(span);
+}
+
+template<typename T, typename U>
+std::span<T> as_span(std::span<U> value) {
+ return std::span{reinterpret_cast<T*>(value.data()), value.size_bytes() / sizeof(T)};
+}
Review Comment:
added warning
--
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 diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1246939583
##########
libminifi/src/utils/LineByLineInputOutputStreamCallback.cpp:
##########
@@ -67,7 +68,7 @@ void LineByLineInputOutputStreamCallback::readLine() {
if (end_of_line != input_.end()) { ++end_of_line; }
current_line_ = next_line_;
- next_line_ = utils::span_to<std::string>(gsl::make_span(&*current_pos_, &*end_of_line).as_span<char>());
+ next_line_ = utils::span_to<std::string>(utils::as_span<char>(std::span(std::to_address(current_pos_), std::to_address(end_of_line))));
Review Comment:
Is `std::to_address` guaranteed to work with vector iterators? I'm not sure iterators are considered "fancy pointers".
--
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] adamdebreceni commented on a diff in pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1584:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584#discussion_r1251932677
##########
libminifi/include/core/repository/AtomicRepoEntries.h:
##########
@@ -18,6 +18,7 @@
#ifndef LIBMINIFI_INCLUDE_CORE_REPOSITORY_ATOMICREPOENTRIES_H_
#define LIBMINIFI_INCLUDE_CORE_REPOSITORY_ATOMICREPOENTRIES_H_
Review Comment:
done
--
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 closed pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits closed pull request #1584: MINIFICPP-1755 - Use std::span instead of gsl::span
URL: https://github.com/apache/nifi-minifi-cpp/pull/1584
--
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