adamdebreceni commented on a change in pull request #839:
URL: https://github.com/apache/nifi-minifi-cpp/pull/839#discussion_r492072291
##########
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:
as far as I know the "nil" uuid is not "invalid", it is described as
only "special"
> The nil UUID is special form of UUID that is specified to have all
128 bits set to zero.
the fact that we treat it as invalid is unfortunate and in the future we
should use `optional<Identifier>` wherever possible, in fact I've already tried
to propagate `optional<Identifier>` but the diff blew up and since it is a
semi-unrelated change I've decided to drop it and take it up in an other ticket
##########
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:
as far as I know the "nil" uuid is not "invalid", it is only described
as "special"
> The nil UUID is special form of UUID that is specified to have all
128 bits set to zero.
the fact that we treat it as invalid is unfortunate and in the future we
should use `optional<Identifier>` wherever possible, in fact I've already tried
to propagate `optional<Identifier>` but the diff blew up and since it is a
semi-unrelated change I've decided to drop it and take it up in an other ticket
##########
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:
I had no intention of making `FlatMap` adhere to any of the named
requirements (that is why it does not), it seems like `AssociativeContainer`
expects an ordering, should we go for `Container`?
(frankly I ~~despise~~ dislike named requirements as they just further push
the responsibilities to the programmer, which IMO should belong to the
compiler, e.g. `Container` expects a `_.cbegin()` with constant complexity, so
what happens if it is not, surely then the compiler rejects the program if we
use the type where a `Container` is expected... in our dreams, mind you this is
not unique to C++)
##########
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:
it can check the existence and signature of methods, although until
C++20 we need something like `std::is_collection`, what it cannot do and will
not do (at least not c++) is check semantic requirements (like the complexity
requirement I mentioned), and worse yet we cannot event express the requirement
in code, so if we want to check that a type satisfies the requirements of
`Collection` we must consult the C++ specification
##########
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:
I'm not against making `FlatMap` generic enough (by making it fulfil
`Container`) for possible future
##########
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:
I'm not against making `FlatMap` generic enough (by making it fulfil
`Container`) for possible future use
----------------------------------------------------------------
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]
