[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r386439462
##
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:
rewritten definition and fixed a possible data race in `IdGenerator`
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r386438667
##
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:
reverted
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r386438350
##
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:
removed `explicit`
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r386437366
##
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:
I've missed this and `HTTPStream::writeData`, but I've found no more
occurences of missing cast.
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r386437675
##
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:
changed 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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385808318
##
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:
Making it copyable in addition to being movable doesn't break the previous
promise. We can keep it non-copyable if desired, but I didn't see the point and
since both declarations were wrong I decided to rely on the implicit copy and
move ctors.
Given the above, do you still want to disable copy or change the
description? (I'd just remove the "movable" word.)
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385805427
##
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:
Pointers are objects. My interpretation of the exception is that the issue
is too minor to be enforced by static analyzers, and is commonly not followed,
hence the "false positives". In other words, my interpretation of the exception
doesn't suggest that function argument objects shouldn't be declared `const`.
I can see the point behind the readability argument, since `const` is
present in the argument list, yet, the string behind the pointer is mutated. I
think the risk of misunderstanding is minor given that we expect people who
view the source to know the language well enough to know what `const` applies
to. With this assumption, IMO the value gained by extra static analysis is
greater than the value of lost readability.
I couldn't find any clear guidelines on the `const`-ness of argument
pointers. I'm willing to remove `const` if you insist on 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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385693436
##
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:
This is inherently specific to Windows, since on windows "hidden" is a file
system attribute while on unix it's a filename prefix. Because of the wildly
different semantics, I would only add it inside an `#ifdef WIN32`/`#endif /*
WIN32 */` there as well.
Would that be good in your opinion?
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385681572
##
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:
I know, but I feel safer with a `< 0` check before making a narrowing cast
(to unsigned) below. (To avoid unsigned vs signed comparison warnings.)
I can revert if you insist on 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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385681572
##
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:
I know, but I feel safer with a `< 0` check before making a narrowing cast
(to unsigned) below. (To avoid unsigned vs signed comparison warnings.)
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385680531
##
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:
It belongs to the definition, not the declaration. It doesn't matter for the
caller but provides extra checks for the definition. I tend to use `const`
whenever it's possible and I don't know a reason not to.
ref1: this slide: https://youtu.be/iz5Qx18H6lg?t=293
ref2:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#con1-by-default-make-objects-immutable
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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385678369 ## 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: Normally when I touch a file, I go through the clang-tidy warnings and apply the suggested fixes. This is one of them. Not strictly necessary, but it takes close to 0 effort and looks nicer. 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] szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm 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_r385677942
##
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:
Yes, the purpose is to silence the warnings about comparison between signed
and unsigned. I think I've applied this universally, but I'll double check.
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
