You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/05/11 15:14:14 UTC

[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

lordgamez opened a new pull request, #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330

   Compile project with clang-14 generating compile_commands.json and use it to analyze code with clang-tidy-14. Clang-tidy is run on every cpp file modified in the pull request. Only an initial batch of checks is implemented, other checks should be implemented as part of [MINIFICPP-1833](https://issues.apache.org/jira/browse/MINIFICPP-1833) or other further tickets.
   
   https://issues.apache.org/jira/browse/MINIFICPP-1832
   
   -----------------------------------------
   Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
   
   In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken:
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced
        in the commit message?
   
   - [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   ### For code changes:
   - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   - [ ] If applicable, have you updated the LICENSE file?
   - [ ] If applicable, have you updated the NOTICE file?
   
   ### For documentation related changes:
   - [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
   
   ### Note:
   Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon 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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872591290


##########
libminifi/test/unit/SocketTests.cpp:
##########
@@ -190,7 +190,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
     std::function<bool()> f_ex = createSocket;
     utils::Worker<bool> functor(f_ex, "id");
     std::future<bool> fut;
-    pool.execute(std::move(functor), fut);
+    pool.execute(std::move(functor), fut);  // NOLINT(bugprone-use-after-move)

Review Comment:
   If we get these false positives, I'd rather disable the check. This is clearly correct usage, and I don't want to have NOLINT everywhere.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872129484


##########
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp:
##########
@@ -887,7 +887,7 @@ void SourceInitiatedSubscriptionListener::onSchedule(const std::shared_ptr<core:
 }
 
 void SourceInitiatedSubscriptionListener::notifyStop() {
-  server_.release();
+  server_.release();  // NOLINT(bugprone-unused-return-value)

Review Comment:
   I think this is, good catch, updated in 3b4102f32349a842f90c11b08189d9e79411eca3



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r871831718


##########
thirdparty/rapidjson-48fbd8cd202ca54031fe799db2ad44ffa8e77c13/include/rapidjson/document.h:
##########
@@ -1236,7 +1236,7 @@ class GenericValue {
 
             // Use static buffer and placement-new to prevent destruction
             static char buffer[sizeof(GenericValue)];
-            return *new (buffer) GenericValue();
+            return *new (buffer) GenericValue();  // NOLINT(clang-analyzer-cplusplus.PlacementNew)

Review Comment:
   Can we exclude the thirdparty directory instead?



##########
libminifi/test/unit/SocketTests.cpp:
##########
@@ -190,7 +190,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
     std::function<bool()> f_ex = createSocket;
     utils::Worker<bool> functor(f_ex, "id");
     std::future<bool> fut;
-    pool.execute(std::move(functor), fut);
+    pool.execute(std::move(functor), fut);  // NOLINT(bugprone-use-after-move)

Review Comment:
   What kind of use-after-move is happening here? This looks perfectly valid to me, not reusing anything after move.



##########
.clang-tidy:
##########
@@ -0,0 +1,2 @@
+Checks: 'boost*,bugprone*,cert*,portability*,-clang-analyzer-optin.cplusplus.VirtualCall,-bugprone-narrowing-conversions,-bugprone-implicit-widening-of-multiplication-result,-bugprone-macro-parentheses,-bugprone-easily-swappable-parameters,-bugprone-exception-escape,-cert-err58-cpp,-cert-err33-c'

Review Comment:
   A few of the disabled ones make sense IMO. This is the list I'm using with CLion:
   ```
   -*
   bugprone-argument-comment
   bugprone-assert-side-effect
   bugprone-bad-signal-to-kill-thread
   bugprone-branch-clone
   bugprone-copy-constructor-init
   bugprone-dangling-handle
   bugprone-dynamic-static-initializers
   bugprone-fold-init-type
   bugprone-forward-declaration-namespace
   bugprone-forwarding-reference-overload
   bugprone-inaccurate-erase
   bugprone-incorrect-roundings
   bugprone-integer-division
   bugprone-lambda-function-name
   bugprone-macro-parentheses
   bugprone-macro-repeated-side-effects
   bugprone-misplaced-operator-in-strlen-in-alloc
   bugprone-misplaced-pointer-arithmetic-in-alloc
   bugprone-misplaced-widening-cast
   bugprone-move-forwarding-reference
   bugprone-multiple-statement-macro
   bugprone-no-escape
   bugprone-not-null-terminated-result
   bugprone-parent-virtual-call
   bugprone-posix-return
   bugprone-reserved-identifier
   bugprone-sizeof-container
   bugprone-sizeof-expression
   bugprone-spuriously-wake-up-functions
   bugprone-string-constructor
   bugprone-string-integer-assignment
   bugprone-string-literal-with-embedded-nul
   bugprone-suspicious-enum-usage
   bugprone-suspicious-include
   bugprone-suspicious-memory-comparison
   bugprone-suspicious-memset-usage
   bugprone-suspicious-missing-comma
   bugprone-suspicious-semicolon
   bugprone-suspicious-string-compare
   bugprone-swapped-arguments
   bugprone-terminating-continue
   bugprone-throw-keyword-missing
   bugprone-too-small-loop-variable
   bugprone-undefined-memory-manipulation
   bugprone-undelegated-constructor
   bugprone-unhandled-self-assignment
   bugprone-unused-raii
   bugprone-unused-return-value
   bugprone-use-after-move
   bugprone-virtual-near-miss
   cert-dcl21-cpp
   cert-dcl58-cpp
   cert-err34-c
   cert-err52-cpp
   cert-err58-cpp
   cert-err60-cpp
   cert-flp30-c
   cert-msc50-cpp
   cert-msc51-cpp
   cert-str34-c
   cppcoreguidelines-interfaces-global-init
   cppcoreguidelines-narrowing-conversions
   cppcoreguidelines-pro-type-member-init
   cppcoreguidelines-pro-type-static-cast-downcast
   cppcoreguidelines-slicing
   google-default-arguments
   google-explicit-constructor
   google-runtime-operator
   hicpp-exception-baseclass
   hicpp-multiway-paths-covered
   misc-misplaced-const
   misc-new-delete-overloads
   misc-no-recursion
   misc-non-copyable-objects
   misc-throw-by-value-catch-by-reference
   misc-unconventional-assign-operator
   misc-uniqueptr-reset-release
   modernize-avoid-bind
   modernize-concat-nested-namespaces
   modernize-deprecated-headers
   modernize-deprecated-ios-base-aliases
   modernize-loop-convert
   modernize-make-shared
   modernize-make-unique
   modernize-pass-by-value
   modernize-raw-string-literal
   modernize-redundant-void-arg
   modernize-replace-auto-ptr
   modernize-replace-disallow-copy-and-assign-macro
   modernize-replace-random-shuffle
   modernize-return-braced-init-list
   modernize-shrink-to-fit
   modernize-unary-static-assert
   modernize-use-auto
   modernize-use-bool-literals
   modernize-use-emplace
   modernize-use-equals-default
   modernize-use-equals-delete
   modernize-use-nodiscard
   modernize-use-noexcept
   modernize-use-nullptr
   modernize-use-override
   modernize-use-transparent-functors
   modernize-use-uncaught-exceptions
   mpi-buffer-deref
   mpi-type-mismatch
   openmp-use-default-none
   performance-faster-string-find
   performance-for-range-copy
   performance-implicit-conversion-in-loop
   performance-inefficient-algorithm
   performance-inefficient-string-concatenation
   performance-inefficient-vector-operation
   performance-move-const-arg
   performance-move-constructor-init
   performance-no-automatic-move
   performance-noexcept-move-constructor
   performance-trivially-destructible
   performance-type-promotion-in-math-fn
   performance-unnecessary-copy-initialization
   performance-unnecessary-value-param
   portability-simd-intrinsics
   readability-avoid-const-params-in-decls
   readability-const-return-type
   readability-container-size-empty
   readability-convert-member-functions-to-static
   readability-delete-null-pointer
   readability-deleted-default
   readability-inconsistent-declaration-parameter-name
   readability-make-member-function-const
   readability-misleading-indentation
   readability-misplaced-array-index
   readability-non-const-parameter
   readability-redundant-control-flow
   readability-redundant-declaration
   readability-redundant-function-ptr-dereference
   readability-redundant-smartptr-get
   readability-redundant-string-cstr
   readability-redundant-string-init
   readability-simplify-subscript-expr
   readability-static-accessed-through-instance
   readability-static-definition-in-anonymous-namespace
   readability-string-compare
   readability-uniqueptr-delete-release
   readability-use-anyofallof
   ```



##########
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp:
##########
@@ -887,7 +887,7 @@ void SourceInitiatedSubscriptionListener::onSchedule(const std::shared_ptr<core:
 }
 
 void SourceInitiatedSubscriptionListener::notifyStop() {
-  server_.release();
+  server_.release();  // NOLINT(bugprone-unused-return-value)

Review Comment:
   Isn't this a real bug? I think it should be reset, not release.



##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   Why this over the spaceship operator?



##########
libminifi/src/controllers/SSLContextService.cpp:
##########
@@ -213,7 +213,8 @@ bool SSLContextService::addP12CertificateToSSLContext(SSL_CTX* ctx) const {
       core::logging::LOG_ERROR(logger_) << "Failed to set additional certificate from " << certificate_ << ", " << getLatestOpenSSLErrorString();
       return false;
     }
-    cacert.release();  // a successful SSL_CTX_add_extra_chain_cert() takes ownership of cacert
+    // a successful SSL_CTX_add_extra_chain_cert() takes ownership of cacert
+    cacert.release();  // NOLINT(bugprone-unused-return-value)

Review Comment:
   Would casting the expression to void hide the warning? It might be an alternative to NOLINT.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r878375796


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   I suggest using memberwise lexicographical comparison in the simplest possible way. AND'ing together the comparison of all members is not consistent: there can be values that are both less and greater than each other, depending on the expression used.
   
   example comparator: https://godbolt.org/z/nz553fbxG



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r886374093


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   Thanks for the workaround! I added your commit to the PR 7c6ee4bbb1d0e7319821d4b5957fc41493b5f3b2



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872128399


##########
.clang-tidy:
##########
@@ -0,0 +1,2 @@
+Checks: 'boost*,bugprone*,cert*,portability*,-clang-analyzer-optin.cplusplus.VirtualCall,-bugprone-narrowing-conversions,-bugprone-implicit-widening-of-multiplication-result,-bugprone-macro-parentheses,-bugprone-easily-swappable-parameters,-bugprone-exception-escape,-cert-err58-cpp,-cert-err33-c'

Review Comment:
   The `cert-err58-cpp` check was disabled because it gave a warning for all `REGISTER_RESOURCE` macro usages and for all initialization of static processor properties (`initialization of '<Property or Processor_registrar>' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]`) and it didn't seem viable to change those.
   
   The `bugprone-macro-parentheses` check was disabled because it gave a warning a huge number of times without any indication of the filename or the line where it was found:
   ```
   warning: macro replacement list should be enclosed in parentheses [bugprone-macro-parentheses]
   ```
   
   As for the other checks I created a seperate Jira ticket [MINIFICPP-1833](https://issues.apache.org/jira/browse/MINIFICPP-1833) to evaluate each check group to see which ones should be integrated and fix the existing issues before adding them to the CI. This jira ticket can still be broken up further if needed, because I think it can effect too many files at once to fix all the issues. But this list is quite useful as integration candidates, thanks!



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872626449


##########
libminifi/test/unit/SocketTests.cpp:
##########
@@ -190,7 +190,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
     std::function<bool()> f_ex = createSocket;
     utils::Worker<bool> functor(f_ex, "id");
     std::future<bool> fut;
-    pool.execute(std::move(functor), fut);
+    pool.execute(std::move(functor), fut);  // NOLINT(bugprone-use-after-move)

Review Comment:
   It would be better, but this false positive only occurs in test code so I would not disable it as it can catch some real problems in production code



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872627252


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   I agree, but if werror is turned on this fails the compilation. What would you suggest?



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r886373092


##########
extensions/libarchive/FocusArchiveEntry.cpp:
##########
@@ -176,15 +176,13 @@ int64_t FocusArchiveEntry::ReadCallback::operator()(const std::shared_ptr<io::Ba
   archive_read_support_filter_all(inputArchive);
 
   // Read each item in the archive
-  int res;
-
-  if ((res = archive_read_open(inputArchive, &data, ok_cb, read_cb, ok_cb))) {
+  if ((archive_read_open(inputArchive, &data, ok_cb, read_cb, ok_cb))) {

Review Comment:
   I think it's not, removed them in e6aea67ca0ead9f8476070cf2d908650baae1b6b



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] adam-markovics commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r877913818


##########
extensions/libarchive/FocusArchiveEntry.cpp:
##########
@@ -176,15 +176,13 @@ int64_t FocusArchiveEntry::ReadCallback::operator()(const std::shared_ptr<io::Ba
   archive_read_support_filter_all(inputArchive);
 
   // Read each item in the archive
-  int res;
-
-  if ((res = archive_read_open(inputArchive, &data, ok_cb, read_cb, ok_cb))) {
+  if ((archive_read_open(inputArchive, &data, ok_cb, read_cb, ok_cb))) {

Review Comment:
   Is double parentheses necessary?



##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   Does it even make sense to say one CpuStatData is greater than another? If it's only for map storage, equality operator and hash would be enough, and turning the map into an unordered one.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r882485407


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   I had some success with a simpler workaround: just define operator<=> when using libc++.
   See the branch on my fork: https://github.com/szaszm/nifi-minifi-cpp/tree/MINIFICPP-1832
   Take the last commit if you like 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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872087728


##########
thirdparty/rapidjson-48fbd8cd202ca54031fe799db2ad44ffa8e77c13/include/rapidjson/document.h:
##########
@@ -1236,7 +1236,7 @@ class GenericValue {
 
             // Use static buffer and placement-new to prevent destruction
             static char buffer[sizeof(GenericValue)];
-            return *new (buffer) GenericValue();
+            return *new (buffer) GenericValue();  // NOLINT(clang-analyzer-cplusplus.PlacementNew)

Review Comment:
   Rapidjson is set to be a system include so it should be exluded by clang-tidy. It does work for every other thirdparty, but somehow when a file uses this inlude clang-tidy still throws a warning about this. Unfortunately I could not find another way of excluding it and it may be due to a clang-tidy-14 bug because when I checked the same files that had this error with clang-tidy-10 I did not get any warnings.



##########
libminifi/test/unit/SocketTests.cpp:
##########
@@ -190,7 +190,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
     std::function<bool()> f_ex = createSocket;
     utils::Worker<bool> functor(f_ex, "id");
     std::future<bool> fut;
-    pool.execute(std::move(functor), fut);
+    pool.execute(std::move(functor), fut);  // NOLINT(bugprone-use-after-move)

Review Comment:
   I don't know either why it gives a warning, it seems okay to me too. I suspected that maybe the TEST_CASE macro does something I'm not aware of, but as it looked okay I only put the NOLINT comment there.



##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   Clang compilation failed with a warning on the spaceship operator in this particular case.
   ```
   /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/procfs/CpuStat.h:37:8: error: explicitly defaulted three-way comparison operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
     auto operator<=>(const CpuStatData& rhs) const = default;
          ^
   /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/procfs/CpuStat.h:57:23: note: defaulted 'operator<=>' is implicitly deleted because there is no viable comparison function for member 'user_'
     SystemClockDuration user_;
   ```
   
   I'm not sure why it can't use it with the SystemClockDuration type.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872129172


##########
libminifi/src/controllers/SSLContextService.cpp:
##########
@@ -213,7 +213,8 @@ bool SSLContextService::addP12CertificateToSSLContext(SSL_CTX* ctx) const {
       core::logging::LOG_ERROR(logger_) << "Failed to set additional certificate from " << certificate_ << ", " << getLatestOpenSSLErrorString();
       return false;
     }
-    cacert.release();  // a successful SSL_CTX_add_extra_chain_cert() takes ownership of cacert
+    // a successful SSL_CTX_add_extra_chain_cert() takes ownership of cacert
+    cacert.release();  // NOLINT(bugprone-unused-return-value)

Review Comment:
   Good idea, updated in 3b4102f32349a842f90c11b08189d9e79411eca3



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r886623725


##########
libminifi/src/FlowControlProtocol.cpp:
##########
@@ -280,11 +280,11 @@ int FlowControlProtocol::sendReportReq() {
   data = this->encode(data, hdr.msgType);
   data = this->encode(data, hdr.seqNumber);
   data = this->encode(data, hdr.status);
-  data = this->encode(data, hdr.payloadLen);
+  this->encode(data, hdr.payloadLen);

Review Comment:
   I think this should not have been changed: now `FLOW_YML_NAME` starts writing the buffer at the same point as where `hdr.payloadLen` started, overwriting 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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872627537


##########
libminifi/test/unit/SocketTests.cpp:
##########
@@ -190,7 +190,7 @@ TEST_CASE("TestTLSContextCreation", "[TestSocket8]") {
     std::function<bool()> f_ex = createSocket;
     utils::Worker<bool> functor(f_ex, "id");
     std::future<bool> fut;
-    pool.execute(std::move(functor), fut);
+    pool.execute(std::move(functor), fut);  // NOLINT(bugprone-use-after-move)

Review Comment:
   OK, fair point.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r872588577


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   This is a clang (or rather libc++) bug. I would rather have one manual spaceship operator implementation that lexicographically compares each member instead of writing all six manually, without any kind of strict ordering.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r878375796


##########
extensions/procfs/CpuStat.h:
##########
@@ -34,7 +34,12 @@ class CpuStatData {
  public:
   static std::optional<CpuStatData> parseCpuStatLine(std::istream& iss);
 
-  auto operator<=>(const CpuStatData& rhs) const = default;
+  bool operator<=(const CpuStatData& rhs) const;
+  bool operator>(const CpuStatData& rhs) const;
+  bool operator>=(const CpuStatData& rhs) const;
+  bool operator<(const CpuStatData& rhs) const;
+  bool operator==(const CpuStatData& rhs) const;
+  bool operator!=(const CpuStatData& rhs) const;

Review Comment:
   I suggest using memberwise lexicographical comparison in the simplest possible way. AND'ing together the comparison of all members is not consistent: there can be values that are both less and greater than each other, depending on the expression used.
   
   example comparator: https://godbolt.org/z/f4nvcET1z



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1330: MINIFICPP-1832 Integrate first batch of clang-tidy checks in CI

Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1330:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1330#discussion_r886647322


##########
libminifi/src/FlowControlProtocol.cpp:
##########
@@ -280,11 +280,11 @@ int FlowControlProtocol::sendReportReq() {
   data = this->encode(data, hdr.msgType);
   data = this->encode(data, hdr.seqNumber);
   data = this->encode(data, hdr.status);
-  data = this->encode(data, hdr.payloadLen);
+  this->encode(data, hdr.payloadLen);

Review Comment:
   Good catch, thanks, reverted in ab2b81f1ca788892c722b8735ee02e13cbedf22c



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org