szaszm commented on a change in pull request #839:
URL: https://github.com/apache/nifi-minifi-cpp/pull/839#discussion_r491104495
##########
File path: libminifi/include/utils/Id.h
##########
@@ -43,107 +44,41 @@ class uuid;
#define UUID_TIME_STR "time"
#define UUID_WINDOWS_STR "windows"
-
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
namespace utils {
-template<typename T, typename C>
-class IdentifierBase {
+class Identifier {
+ friend struct IdentifierTestAccessor;
public:
- IdentifierBase(T myid) { // NOLINT
- copyInto(myid);
- }
-
- IdentifierBase(const IdentifierBase &other) {
- copyInto(other.id_);
- }
-
- IdentifierBase(IdentifierBase &&other)
- : converted_(std::move(other.converted_)) {
- copyInto(other.id_);
- }
-
- IdentifierBase() = default;
-
- IdentifierBase &operator=(const IdentifierBase &other) {
- copyInto(other.id_);
- return *this;
- }
-
- IdentifierBase &operator=(T o) {
- copyInto(o);
- return *this;
- }
+ using Data = std::array<uint8_t, 16>;
- void getIdentifier(T other) const {
- copyOutOf(other);
- }
+ Identifier() = default;
+ explicit Identifier(const Data& data);
+ explicit Identifier(const std::string& data);
+ Identifier &operator=(const Data& data);
- C convert() const {
- return converted_;
- }
+ Identifier &operator=(const std::string& idStr);
- protected:
- void copyInto(const IdentifierBase &other) {
- memcpy(id_, other.id_, sizeof(T));
- }
+ bool operator!=(const Identifier& other) const;
+ bool operator==(const Identifier& other) const;
- void copyInto(const void *other) {
- memcpy(id_, other, sizeof(T));
- }
-
- void copyOutOf(void *other) {
- memcpy(other, id_, sizeof(T));
- }
-
- C converted_{};
-
- T id_{};
-};
-
-typedef uint8_t UUID_FIELD[16];
-
-class Identifier : public IdentifierBase<UUID_FIELD, std::string> {
- public:
- Identifier(UUID_FIELD u); // NOLINT
- Identifier();
- Identifier(const Identifier &other);
- Identifier(Identifier &&other);
-
- /**
- * I believe these exist to make windows builds happy -- need more testing
- * to ensure this doesn't cause any issues.
- */
- Identifier(const IdentifierBase &other); // NOLINT
- Identifier &operator=(const IdentifierBase &other);
-
- Identifier &operator=(const Identifier &other);
- Identifier &operator=(UUID_FIELD o);
-
- Identifier &operator=(std::string id);
- bool operator==(const std::nullptr_t nullp) const;
-
- bool operator!=(std::nullptr_t nullp) const;
-
- bool operator!=(const Identifier &other) const;
- bool operator==(const Identifier &other) const;
+ bool isNil() const;
Review comment:
How about an `explicit operator bool()`?
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-if
> Often, if (p) is read as “if p is valid” which is a direct expression of
the programmers intent
##########
File path: libminifi/include/utils/Id.h
##########
@@ -43,107 +44,41 @@ class uuid;
#define UUID_TIME_STR "time"
#define UUID_WINDOWS_STR "windows"
-
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
namespace utils {
-template<typename T, typename C>
-class IdentifierBase {
+class Identifier {
+ friend struct IdentifierTestAccessor;
public:
- IdentifierBase(T myid) { // NOLINT
- copyInto(myid);
- }
-
- IdentifierBase(const IdentifierBase &other) {
- copyInto(other.id_);
- }
-
- IdentifierBase(IdentifierBase &&other)
- : converted_(std::move(other.converted_)) {
- copyInto(other.id_);
- }
-
- IdentifierBase() = default;
-
- IdentifierBase &operator=(const IdentifierBase &other) {
- copyInto(other.id_);
- return *this;
- }
-
- IdentifierBase &operator=(T o) {
- copyInto(o);
- return *this;
- }
+ using Data = std::array<uint8_t, 16>;
- void getIdentifier(T other) const {
- copyOutOf(other);
- }
+ Identifier() = default;
+ explicit Identifier(const Data& data);
+ explicit Identifier(const std::string& data);
Review comment:
I would even disable the string interface and have the callers use the
public `parse`.
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
Review comment:
```suggestion
using value_type = std::pair<K, V>;
using reference = value_type&;
using const_reference = const value_type&;
using difference_type = typename std::vector<value_type>::difference_type;
using size_type = typename std::vector<value_type>::size_type;
```
also, [Container](https://en.cppreference.com/w/cpp/named_req/Container)
requires `cbegin`/`cend`, `operator==`/`operator!=`, `swap` (both member and
free-function), `max_size`, `empty` and that the iterator types satisfy
[ForwardIterator](https://en.cppreference.com/w/cpp/named_req/ForwardIterator)
(stronger than InputIterator).
You may also want to look at
[AssociativeContainer](https://en.cppreference.com/w/cpp/named_req/AssociativeContainer).
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
+
+ private:
+ using Container = std::vector<value_type>;
+
+ public:
+ class iterator{
+ friend class const_iterator;
+ friend class FlatMap;
+ explicit iterator(typename Container::iterator it): it_(it) {}
+
+ public:
+ using difference_type = void;
+ using value_type = FlatMap::value_type;
+ using pointer = void;
+ using reference = void;
+ using iterator_category = void;
+
+ value_type* operator->() const {return &(*it_);}
+ value_type& operator*() const {return *it_;}
+
+ bool operator==(const iterator& other) const {
+ return it_ == other.it_;
+ }
+
+ bool operator!=(const iterator& other) const {
+ return !(*this == other);
+ }
+
+ iterator& operator++() {
+ ++it_;
+ return *this;
+ }
+
+ private:
+ typename Container::iterator it_;
+ };
+
+ class const_iterator{
+ friend class FlatMap;
+ explicit const_iterator(typename Container::const_iterator it): it_(it) {}
+
+ public:
+ const_iterator(iterator it): it_(it.it_) {} // NOLINT
+ using difference_type = void;
+ using value_type = const FlatMap::value_type;
+ using pointer = void;
+ using reference = void;
+ using iterator_category = void;
+
+ value_type* operator->() const {return &(*it_);}
+ value_type& operator*() const {return *it_;}
+
+ bool operator==(const const_iterator& other) const {
+ return it_ == other.it_;
+ }
+
+ bool operator!=(const const_iterator& other) const {
+ return !(*this == other);
+ }
+
+ const_iterator& operator++() {
+ ++it_;
+ return *this;
+ }
+
+ private:
+ typename Container::const_iterator it_;
+ };
+
+ FlatMap() = default;
+ FlatMap(const FlatMap&) = default;
+ FlatMap(FlatMap&&) noexcept = default;
+ FlatMap(std::initializer_list<value_type> items) : data_(items) {}
+ template<class InputIterator>
+ FlatMap(InputIterator begin, InputIterator end) : data_(begin, end) {}
+
+ FlatMap& operator=(const FlatMap& source) = default;
+ FlatMap& operator=(FlatMap&& source) = default;
+ FlatMap& operator=(std::initializer_list<value_type> items) {
+ data_ = items;
+ return *this;
+ }
+
+ std::size_t size() {
+ return data_.size();
+ }
+
+ V& operator[](const K& key) {
+ auto it = find(key);
+ if (it != end()) {
+ return it->second;
+ }
+ data_.emplace_back(key, V{});
+ return data_.rbegin()->second;
+ }
+
+ iterator erase(const_iterator pos) {
+ auto offset = pos.it_ - data_.begin();
+ std::swap(*data_.rbegin(), *(data_.begin() + offset));
+ data_.pop_back();
+ return iterator{data_.begin() + offset};
+ }
+
+ std::size_t erase(const K& key) {
+ for (auto it = data_.begin(); it != data_.end(); ++it) {
+ if (it->first == key) {
+ std::swap(*data_.rbegin(), *it);
+ data_.pop_back();
+ return 1;
+ }
+ }
+ return 0;
+ }
+
+ std::pair<iterator, bool> insert(const value_type& value) {
+ auto it = find(value.first);
+ if (it != end()) {
+ return {it, false};
+ }
+ data_.push_back(value);
+ return {iterator{data_.begin() + data_.size() - 1}, true};
+ }
+
+ template<typename M>
+ std::pair<iterator, bool> insert_or_assign(const K& key, M&& value) {
+ auto it = find(key);
+ if (it != end()) {
+ it->second = std::forward<M>(value);
+ return {it, false};
+ }
+ data_.emplace_back(key, std::forward<M>(value));
+ return {iterator{data_.begin() + data_.size() - 1}, true};
+ }
+
+ iterator find(const K& key) {
+ for (auto it = data_.begin(); it != data_.end(); ++it) {
+ if (it->first == key) return iterator{it};
+ }
+ return end();
+ }
+
+ const_iterator find(const K& key) const {
+ for (auto it = data_.begin(); it != data_.end(); ++it) {
+ if (it->first == key) return const_iterator{it};
+ }
+ return end();
+ }
+
+ iterator begin() {
+ return iterator{data_.begin()};
+ }
+
+ iterator end() {
+ return iterator{data_.end()};
+ }
+
+ const_iterator begin() const {
+ return const_iterator{data_.begin()};
+ }
+
+ const_iterator end() const {
+ return const_iterator{data_.end()};
+ }
Review comment:
```suggestion
}
const_iterator cbegin() const noexcept {
return begin();
}
const_iterator cend() const noexcept {
return end();
}
bool empty() const noexcept {
return begin() == end();
}
size_type max_size() const noexcept {
return data_.max_size();
}
```
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
+
+ private:
+ using Container = std::vector<value_type>;
+
+ public:
+ class iterator{
+ friend class const_iterator;
+ friend class FlatMap;
+ explicit iterator(typename Container::iterator it): it_(it) {}
+
+ public:
+ using difference_type = void;
+ using value_type = FlatMap::value_type;
+ using pointer = void;
+ using reference = void;
+ using iterator_category = void;
Review comment:
```suggestion
using difference_type = typename Container::iterator::difference_type;
using value_type = FlatMap::value_type;
using pointer = FlatMap::value_type*;
using reference = FlatMap::value_type&;
using iterator_category = std::input_iterator_tag;
```
[InputIterator](https://en.cppreference.com/w/cpp/named_req/InputIterator)
requires post-increment operator as well.
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
+
+ private:
+ using Container = std::vector<value_type>;
+
+ public:
+ class iterator{
+ friend class const_iterator;
+ friend class FlatMap;
+ explicit iterator(typename Container::iterator it): it_(it) {}
+
+ public:
+ using difference_type = void;
+ using value_type = FlatMap::value_type;
+ using pointer = void;
+ using reference = void;
+ using iterator_category = void;
+
+ value_type* operator->() const {return &(*it_);}
+ value_type& operator*() const {return *it_;}
+
+ bool operator==(const iterator& other) const {
+ return it_ == other.it_;
+ }
+
+ bool operator!=(const iterator& other) const {
+ return !(*this == other);
+ }
+
+ iterator& operator++() {
+ ++it_;
+ return *this;
+ }
+
+ private:
+ typename Container::iterator it_;
+ };
+
+ class const_iterator{
+ friend class FlatMap;
+ explicit const_iterator(typename Container::const_iterator it): it_(it) {}
+
+ public:
+ const_iterator(iterator it): it_(it.it_) {} // NOLINT
+ using difference_type = void;
+ using value_type = const FlatMap::value_type;
+ using pointer = void;
+ using reference = void;
+ using iterator_category = void;
Review comment:
```suggestion
using difference_type = typename Container::iterator::difference_type;
using value_type = const FlatMap::value_type;
using pointer = const FlatMap::value_type*;
using reference = const FlatMap::value_type&;
using iterator_category = std::input_iterator_tag;
```
##########
File path: libminifi/src/core/FlowFile.cpp
##########
@@ -43,16 +45,13 @@ FlowFile::FlowFile()
event_time_(0),
claim_(nullptr),
marked_delete_(false),
- connection_(nullptr),
- original_connection_() {
+ connection_() {
Review comment:
Unnecessary line.
##########
File path: libminifi/src/FlowFileRecord.cpp
##########
@@ -261,88 +144,104 @@ bool FlowFileRecord::Serialize() {
return false;
}
- if (flow_repository_->Put(uuidStr_,
const_cast<uint8_t*>(outStream.getBuffer()), outStream.getSize())) {
- logger_->log_debug("NiFi FlowFile Store event %s size %llu success",
uuidStr_, outStream.getSize());
+ if (flowRepository->Put(getUUIDStr(),
const_cast<uint8_t*>(outStream.getBuffer()), outStream.getSize())) {
+ logger_->log_debug("NiFi FlowFile Store event %s size " "%" PRIu64 "
success", getUUIDStr(), outStream.getSize());
// on behalf of the persisted record instance
if (claim_) claim_->increaseFlowFileRecordOwnedCount();
return true;
} else {
- logger_->log_error("NiFi FlowFile Store event %s size %llu fail",
uuidStr_, outStream.getSize());
+ logger_->log_error("NiFi FlowFile Store failed %s size " "%" PRIu64 "
fail", getUUIDStr(), outStream.getSize());
Review comment:
```suggestion
logger_->log_error("NiFi FlowFile Store failed %s size " "%" PRIu64,
getUUIDStr(), outStream.getSize());
```
Since "event" is changed to "failed", the second "fail" is no longer
necessary.
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
Review comment:
My point is that if you write a container, then it should behave like a
canonical container so that it can work as a drop-in replacement for at least
some containers. Named requirements (i.e. concepts) just clarify the properties
of a canonical container so that implementors have guidelines to follow and
users have an interface to code against. They are just the generic programming
equivalent of an interface, with less dependencies and less opportunity for the
compiler to check whether the requirements are satisfied. You can not implement
`java.util.Collection` without implementing `isEmpty`, but the compiler can not
yet enforce that your Collection implementation has `empty`.
I think the closer the satisfied concepts are to the reality, the better.
Ideally, this would satisfy AssociativeContainer and iterators would satisfy
RandomAccessIterator, but I'm OK with postponing the implementation of stricter
requirements until they are actually needed.
##########
File path: libminifi/include/utils/FlatMap.h
##########
@@ -0,0 +1,211 @@
+/**
+ *
+ * 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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+#define LIBMINIFI_INCLUDE_UTILS_FLATMAP_H_
+
+#include <tuple>
+#include <functional>
+#include <vector>
+#include <utility>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template<typename K, typename V>
+class FlatMap{
+ public:
+ using value_type = std::pair<K, V>;
Review comment:
Alternatively, we can leave it as non-generic and hide the
implementation as an implementation detail of `FlowFile`, but we would lose
software capital by choosing that route.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
