[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386298703
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
Our core code is expected to be written by people very familiar with the
language. We strive to create an ecosystem where extensions can be written (our
API can be used) safely even by people without great familiarity with the
language.
This change introduces a risk of misunderstanding by someone less familiar
with the language (or someone familiar with it, just running low on caffeine),
without bringing comparable benefits.
Please revert it.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386299652
##
File path: libminifi/include/utils/BackTrace.h
##
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string )
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
I am OK with it being copyable, but then the description should be changed.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386298703
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
Our core code is expected to be written by people very familiar with the
language. We strive to create an ecosystem where extensions can be written (our
API can be used) safely even by people without great familiarity with the
language.
This change introduces a risk of misunderstanding by someone less familiar
with a language (or someone familiar with it, just running low on caffeine),
without bringing comparable benefits.
Please revert it.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385773769
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
@szaszm my bad, your second reference actually addresses the issue, and
spells out the reasons I give not to use it in this case:
```Exception Function arguments are rarely mutated, but also rarely declared
const. To avoid confusion and lots of false positives, don’t enforce this rule
for function arguments.```
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385771963 ## File path: extensions/standard-processors/processors/ExtractText.cpp ## @@ -121,7 +122,7 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr strea ctx_->getProperty(SizeLimit.getName(), sizeLimitStr); ctx_->getProperty(RegexMode.getName(), regex_mode); - if (sizeLimitStr == "") + if (sizeLimitStr.empty()) Review comment: I don't agree with it looking nicer, but I can live with it. 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 With regards, Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385771605
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
I understand your intent, but it can be confusing for users when looking at
the function signature. I agree with making as many things const as possible,
but this, in my opinion, reduces readability of the API without having much
benefit.
None of your references specifically address the case of having const
pointers in function signatures, they are just speaking generically about
immutable objects - and as I said, in the generic case, I agree.
Neither do I know of any major projects that would use const pointers on
their API.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385768427
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -640,10 +637,10 @@ class FileUtils {
std::vector buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
It doesn't hurt, I just didn't understand the purpose, but I can live with
it.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385701729
##
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+std::stringstream ss;
+ss << dir << utils::file::FileUtils::get_separator() <<
".filewithoutanext";
+return ss.str();
+ }();
+ {
+std::ofstream file{ hidden_file_name };
+file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
Yeah, that's fine.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242227
##
File path: libminifi/include/utils/BackTrace.h
##
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string )
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
This was messed up, but the class description says that `Backtrace is a
movable vector of trace lines.`, so either we should make it noncopyable, or
justify why we need to copy it and change the description.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385257180
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -640,10 +637,10 @@ class FileUtils {
std::vector buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
`readlink`'s contract is pretty clear from the POSIX specification
(https://pubs.opengroup.org/onlinepubs/009695399/functions/readlink.html): it
will return -1 on error, not negative on error. This is fine the way it is.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385238986
##
File path: libminifi/include/core/Repository.h
##
@@ -127,7 +127,7 @@ class Repository : public virtual
core::SerializableComponent, public core::Trac
/**
* Since SerializableComponents represent a runnable object, we should
return traces
*/
- virtual BackTrace &() {
+ virtual BackTrace getTraces() {
Review 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385246943
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
What is the purpose of using a const pointer (not pointer to const) here as
a function argument? It gives no extra guarantees to the caller.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385231359 ## File path: extensions/standard-processors/processors/ExtractText.cpp ## @@ -121,7 +122,7 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr strea ctx_->getProperty(SizeLimit.getName(), sizeLimitStr); ctx_->getProperty(RegexMode.getName(), regex_mode); - if (sizeLimitStr == "") + if (sizeLimitStr.empty()) Review comment: I don't mind `empty()` too much, but why was this necessary? `std::string` has a pretty effective `operator==` with `const CharT* rhs`. 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 With regards, Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385234979
##
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+std::stringstream ss;
+ss << dir << utils::file::FileUtils::get_separator() <<
".filewithoutanext";
+return ss.str();
+ }();
+ {
+std::ofstream file{ hidden_file_name };
+file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
I would prefer if you could add this to `utils::file::FileUtils`, we might
need it again.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242227
##
File path: libminifi/include/utils/BackTrace.h
##
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string )
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
This was messed up, put the class description says that `Backtrace is a
movable vector of trace lines.`, so either we should make it noncopyable, or
justify why we need to copy it and change the description.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385254493
##
File path: libminifi/include/utils/file/FileUtils.h
##
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
#ifdef WIN32
- std::string tempDirectory;
- char tempBuffer[MAX_PATH];
- auto ret = GetTempPath(MAX_PATH, tempBuffer);
- if (ret <= MAX_PATH && ret != 0)
- {
- static std::shared_ptr generator;
- if (!generator) {
- generator =
minifi::utils::IdGenerator::getIdGenerator();
-
generator->initialize(std::make_shared());
- }
- tempDirectory = tempBuffer;
- minifi::utils::Identifier id;
- generator->generate(id);
- tempDirectory += id.to_string();
- create_dir(tempDirectory);
- }
- return tempDirectory;
+std::string tempDirectory;
+char tempBuffer[MAX_PATH];
+auto ret = GetTempPath(MAX_PATH, tempBuffer);
+if (ret <= MAX_PATH && ret != 0)
+{
+ static std::shared_ptr generator;
Review comment:
I know this is old code, but it is horrifically unsafe. Since it is
currently only used from tests I don't mind it so much, but:
- `minifi::utils::IdGenerator::getIdGenerator` is already a singleton
getter so this does function static variable does not make much sense aside
from saving a shared_ptr copy
- only the creation (and initialization to nullptr) of `generator` is
protected, the check for nullptr and assignment from
`minifi::utils::IdGenerator::getIdGenerator` is not, so it is a race condition
- `generator->initialize` would not be protected even if `generator` would
be initially initialized to `minifi::utils::IdGenerator::getIdGenerator()`,
that's why `generator->initialize` should only be called at the beginning of a
main executable, when there is only a single thread (see `main/MiNiFiMain.cpp`
and `libminifi/test/TestBase.h`)
This whole part should just be rewritten to
`utils::IdGenerator::getIdGenerator()->generate(id)`, and if it breaks
something, that's the real problem.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242526
##
File path: libminifi/include/utils/BackTrace.h
##
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string )
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
Review comment:
`explicit` breaks API.
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385227484
##
File path: extensions/http-curl/client/HTTPStream.cpp
##
@@ -90,13 +95,17 @@ inline std::vector HttpStream::readBuffer(const
T& t) {
}
int HttpStream::readData(std::vector , int buflen) {
- if ((int) buf.capacity() < buflen) {
+ if (buflen < 0) {
+throw minifi::Exception{ExceptionType::GENERAL_EXCEPTION, "negative
buflen"};
+ }
+
+ if (buf.size() < static_cast(buflen)) {
Review comment:
If this cast is the way to silence a warning I am OK with it, but then we
should do the same in `HttpStream::writeData` (and all the other
`readData`/`writeData` functions modified here).
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385233811
##
File path: extensions/standard-processors/processors/ExtractText.cpp
##
@@ -212,10 +213,10 @@ int64_t
ExtractText::ReadCallback::process(std::shared_ptr strea
}
ExtractText::ReadCallback::ReadCallback(std::shared_ptr
flowFile, core::ProcessContext *ctx, std::shared_ptr lgr)
-: flowFile_(flowFile),
+: flowFile_(std::move(flowFile)),
ctx_(ctx),
- logger_(lgr) {
- buffer_.reserve(std::min(flowFile->getSize(), MAX_BUFFER_SIZE));
+ logger_(std::move(lgr)) {
+ buffer_.resize(std::min(flowFile_->getSize(), MAX_BUFFER_SIZE));
Review 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
With regards,
Apache Git Services
