[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r509320738
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r509166507
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
@arpadboda are you too okay with this solution?
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r509027132
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
previously I wrote:
> I generally like if the types convey their usage, so we can have functions
expecting `UUIDString`
this would make sense if we wanted functions expecting a `UUIDString`, I
argue that we don't and wherever we pass/store a stringified `Identifier` we
instead should pass/store an `Identifier`,
[this](https://issues.apache.org/jira/browse/MINIFICPP-1395) is the issue
aiming to make this change, where I think `UUIDString` would be removed
in light of this I would remove the `UUIDString` typedef
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r508274717
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
added a comment
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r508247054
##
File path: libminifi/src/core/ProcessSession.cpp
##
@@ -66,10 +66,12 @@ ProcessSession::~ProcessSession() {
}
void ProcessSession::add(const std::shared_ptr ) {
- if (_updatedFlowFiles.find(record->getUUIDStr()) != _updatedFlowFiles.end())
{
+ utils::Identifier uuid;
+ record->getUUID(uuid);
+ if (_updatedFlowFiles.find(uuid) != _updatedFlowFiles.end()) {
Review comment:
you are right, whatever we consider `Nil` to be (invalid or valid), the
`getUUID` implementation is the same, created a ticket for refactoring our
`Identifier` treatment e.g. don't pass `.getUUIDStr()` when a `.getUUID()`
would be perfectly fine, and to replace all `.getUUID(uuid)` with the new
implementation, I imagine there are quite a number of places that will be
touched by that PR
the created ticket is
[here](https://issues.apache.org/jira/browse/MINIFICPP-1395)
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507761664
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
on second thought, this whole "pass identifiers as strings" interface
needs a rethinking, since the only place we should call `Identifier::to_string`
should be when we create logs or other outputs for human consumption
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507766370
##
File path: libminifi/src/core/ProcessSession.cpp
##
@@ -66,10 +66,12 @@ ProcessSession::~ProcessSession() {
}
void ProcessSession::add(const std::shared_ptr ) {
- if (_updatedFlowFiles.find(record->getUUIDStr()) != _updatedFlowFiles.end())
{
+ utils::Identifier uuid;
+ record->getUUID(uuid);
+ if (_updatedFlowFiles.find(uuid) != _updatedFlowFiles.end()) {
Review comment:
the jury is still out on if we should consider `Nil` an `invalid` uuid,
in the spec it is only mentioned as "special", but I am all for restricting our
set of valid uuid's to not include `Nil` and then we can abolish all the `bool
getUUID(Identifier&)` stuff
(the `getUUID` implementation itself already considers `Nil` to be invalid,
so we just have to commit to this direction)
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507760547
##
File path: libminifi/include/utils/SmallString.h
##
@@ -0,0 +1,86 @@
+/**
+ * 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
+#include
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template
+class SmallString : public std::array {
+ public:
+ operator std::string() const { // NOLINT
Review comment:
on a practical note, there are quite a number of places passing the
result of `getUUIDStr()` where a `std::string` is expected, IMO we should
rewrite all these functions to expect an `Identifier`
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507761664
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
on a second thought, this whole "pass identifiers as strings" interface
needs a rethinking, since the only place we should call `Identifier::to_string`
should be when we create logs or other outputs for human consumption
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507760547
##
File path: libminifi/include/utils/SmallString.h
##
@@ -0,0 +1,86 @@
+/**
+ * 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
+#include
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template
+class SmallString : public std::array {
+ public:
+ operator std::string() const { // NOLINT
Review comment:
on a practical note, there are quite a number of places passing
`::getUUIDStr()` where a `std::string` is expected, IMO we should rewrite all
these functions to expect an `Identifier`
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507758324
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
I generally like if the types convey their usage, so we can have
functions expecting `UUIDString` and we will make no mistake passing it a
`"banana"` (the intent of the `Identifier::to_string` is not to return a
`std::string` but to stringify its value, and that representation is quite
constrained compared to `std::string`)
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507752703
##
File path: libminifi/include/utils/SmallString.h
##
@@ -0,0 +1,86 @@
+/**
+ * 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
+#include
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template
+class SmallString : public std::array {
+ public:
+ operator std::string() const { // NOLINT
Review comment:
in this case I would keep it implicit since using `SmallString` is
purely an optimization and it should be kept as transparent 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507746211
##
File path: libminifi/include/utils/SmallString.h
##
@@ -0,0 +1,86 @@
+/**
+ * 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
+#include
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template
+class SmallString : public std::array {
+ public:
+ operator std::string() const { // NOLINT
+return {this->data()};
+ }
+
Review comment:
added c_str method, and using it instead of array<>::data wherever
applicable
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507746211
##
File path: libminifi/include/utils/SmallString.h
##
@@ -0,0 +1,86 @@
+/**
+ * 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
+#include
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template
+class SmallString : public std::array {
+ public:
+ operator std::string() const { // NOLINT
+return {this->data()};
+ }
+
Review comment:
added c_str method, and using it instead of data wherever applicable
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #926: MINIFICPP-1387 - Speed up Identifier to string conversion
adamdebreceni commented on a change in pull request #926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/926#discussion_r507745780
##
File path: libminifi/include/utils/Id.h
##
@@ -65,10 +67,11 @@ class Identifier {
bool operator!=(const Identifier& other) const;
bool operator==(const Identifier& other) const;
+ bool operator<(const Identifier& other) const;
bool isNil() const;
- std::string to_string() const;
+ SmallString<36> to_string() const;
Review comment:
added UUIDString alias
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:
us...@infra.apache.org
