tools,src: remove clang-format #42665

Trott · 2022-04-09T00:28:18Z

The clang-format tools generates code that is at odds with cpplint.py.
Since we run cpplint.py on every commit and (apparently) almost never
run clang-format, let's simplify things by removing clang-format.

The clang-format tools generates code that is at odds with cpplint.py. Since we run cpplint.py on every commit and (apparently) almost never run clang-format, let's simplify things by removing clang-format.

nodejs-github-bot · 2022-04-09T00:44:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43403/

targos

I rarely write C++ code, but having a formatter is very useful to me for this language

RaisinTen · 2022-04-09T07:48:10Z

The clang-format tools generates code that is at odds with cpplint.py.

Do you have an example diff? I thought that clang-format is like a stricter form of cpplint but both are supposed to adhere to the same set of rules in our C++ style guide. If they produce conflicting results, we are supposed to tweak the configs at https://github.com/nodejs/node/blob/HEAD/.clang-format and https://github.com/nodejs/node/blob/HEAD/.cpplint accordingly.

Since we run cpplint.py on every commit and (apparently) almost never
run clang-format, let's simplify things by removing clang-format.

I think it would be really nice if we can run both of them.

Trott · 2022-04-09T11:01:28Z

Do you have an example diff?

$ make format-cpp-build
...
$ CLANG_FORMAT_START=9d7895c567e8f38abfff35da1b6d6d6a0a06f9aa make format-cpp
...
$ make lint-cpp
Running C++ linter...
src/api/embed_helpers.cc:129:  Use left leaning pointer instead of right leaning  [readability/pointer_notation] [2]
Done processing src/api/embed_helpers.cc
src/crypto/crypto_bio.cc:154:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
Done processing src/crypto/crypto_bio.cc
src/crypto/crypto_bio.h:133:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
Done processing src/crypto/crypto_bio.h
src/crypto/crypto_context.cc:492:  Use left leaning pointer instead of right leaning  [readability/pointer_notation] [2]
Done processing src/crypto/crypto_context.cc
src/env.cc:480:  Use left leaning pointer instead of right leaning  [readability/pointer_notation] [2]
Done processing src/env.cc
src/node.h:83:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
src/node.h:91:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
src/node.h:871:  Use nullptr instead of NULL  [readability/null_usage] [2]
Done processing src/node.h
src/node_sockaddr.cc:1:  src/node_sockaddr.cc should include its header file src/node_sockaddr.h  [build/include] [5]
Done processing src/node_sockaddr.cc
src/node_version.h:69:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
src/node_version.h:70:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
Done processing src/node_version.h
src/node_worker.cc:204:  Use left leaning pointer instead of right leaning  [readability/pointer_notation] [2]
Done processing src/node_worker.cc
src/stream_base-inl.h:75:  Line contains only semicolon. If this should be an empty statement, use {} instead.  [whitespace/semicolon] [5]
Done processing src/stream_base-inl.h
test/addons/openssl-client-cert-engine/testengine.cc:53:  Use left leaning pointer instead of right leaning  [readability/pointer_notation] [2]
Done processing test/addons/openssl-client-cert-engine/testengine.cc
Total errors found: 14
make: *** [tools/.cpplintstamp] Error 1
$

Trott · 2022-04-09T11:27:47Z

I see in a few places that the problem is that a line with a // NOLINT comment is split into multiple lines, and so the comment isn't always on a line that needs it. That at least should be pretty easy to fix.

Refs: nodejs#42665 (comment)

Run clang-format on the files and provide needed additional comments for the linter. Refs: nodejs#42665 (comment)

Trott · 2022-04-09T11:43:55Z

I see in a few places that the problem is that a line with a // NOLINT comment is split into multiple lines, and so the comment isn't always on a line that needs it. That at least should be pretty easy to fix.

#42668

RaisinTen · 2022-04-09T11:49:16Z

Managed to come up with a diff to make lint-cpp happy but the [readability/pointer_notation] ones look like mistakes to me.

Details

diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc
index 6ca63af36f..786c8d8cec 100644
--- a/src/api/embed_helpers.cc
+++ b/src/api/embed_helpers.cc
@@ -126,7 +126,7 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() {
     bool platform_finished = false;
     impl_->platform->AddIsolateFinishedCallback(
         isolate,
-        [](void* data) { *static_cast<bool*>(data) = true; },
+        [](void* data) { (*static_cast<bool*>(data)) = true; },
         &platform_finished);
     impl_->platform->UnregisterIsolate(isolate);
     isolate->Dispose();
diff --git a/src/crypto/crypto_bio.cc b/src/crypto/crypto_bio.cc
index a4bae59091..0f6610cffc 100644
--- a/src/crypto/crypto_bio.cc
+++ b/src/crypto/crypto_bio.cc
@@ -151,7 +151,7 @@ int NodeBIO::Gets(BIO* bio, char* out, int size) {
   return i;
 }
 
-long NodeBIO::Ctrl(BIO* bio,
+long NodeBIO::Ctrl(BIO* bio,  // NOLINT(runtime/int)
                    int cmd,
                    long num,  // NOLINT(runtime/int)
                    void* ptr) {
diff --git a/src/crypto/crypto_bio.h b/src/crypto/crypto_bio.h
index f087ea8c83..f774e6f464 100644
--- a/src/crypto/crypto_bio.h
+++ b/src/crypto/crypto_bio.h
@@ -130,7 +130,7 @@ class NodeBIO : public MemoryRetainer {
   static int Write(BIO* bio, const char* data, int len);
   static int Puts(BIO* bio, const char* str);
   static int Gets(BIO* bio, char* out, int size);
-  static long Ctrl(BIO* bio,
+  static long Ctrl(BIO* bio,  // NOLINT(runtime/int)
                    int cmd,
                    long num,  // NOLINT(runtime/int)
                    void* ptr);
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 364c1a4102..4b81c5894f 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -489,7 +489,7 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
       method = TLS_client_method();
     } else {
       const std::string msg("Unknown method: ");
-      THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, (msg + *sslmethod).c_str());
+      THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, (msg + (*sslmethod)).c_str());
       return;
     }
   }
diff --git a/src/env.cc b/src/env.cc
index ceb8d3ff1b..97f783fb82 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -477,7 +477,7 @@ Environment::~Environment() {
 #ifdef DEBUG
     bool consistency_check = false;
     isolate()->RequestInterrupt(
-        [](Isolate*, void* data) { *static_cast<bool*>(data) = true; },
+        [](Isolate*, void* data) { (*static_cast<bool*>(data)) = true; },
         &consistency_check);
 #endif
 
diff --git a/src/node.h b/src/node.h
index f4829d0a11..e4bde37d0e 100644
--- a/src/node.h
+++ b/src/node.h
@@ -80,6 +80,7 @@
 #ifdef __clang__
 #define NODE_CLANG_AT_LEAST(major, minor, patch)                               \
   (NODE_MAKE_VERSION(major, minor, patch) <=                                   \
+  /* NOLINTNEXTLINE(whitespace/indent) */                                      \
    NODE_MAKE_VERSION(__clang_major__, __clang_minor__, __clang_patchlevel__))
 #else
 #define NODE_CLANG_AT_LEAST(major, minor, patch) (0)
@@ -88,6 +89,7 @@
 #ifdef __GNUC__
 #define NODE_GNUC_AT_LEAST(major, minor, patch)                                \
   (NODE_MAKE_VERSION(major, minor, patch) <=                                   \
+  /* NOLINTNEXTLINE(whitespace/indent) */                                      \
    NODE_MAKE_VERSION(__GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__))
 #else
 #define NODE_GNUC_AT_LEAST(major, minor, patch) (0)
@@ -866,8 +868,8 @@ extern "C" NODE_EXTERN void node_module_register(void* mod);
 // It is used the same way addon bindings are used, except that linked bindings
 // can be accessed through `process._linkedBinding(modname)`.
 #define NODE_MODULE_LINKED(modname, regfunc)                                   \
-  /* NOLINTNEXTLINE (readability/null_usage) */                                \
   NODE_MODULE_CONTEXT_AWARE_X(                                                 \
+  /* NOLINTNEXTLINE (readability/null_usage) */                                \
       modname, regfunc, NULL, node::ModuleFlags::kLinked)
 
 /*
diff --git a/src/node_sockaddr.cc b/src/node_sockaddr.cc
index 86858aba8f..c94c4a3e40 100644
--- a/src/node_sockaddr.cc
+++ b/src/node_sockaddr.cc
@@ -1,9 +1,10 @@
+#include "node_sockaddr-inl.h"  // NOLINT(build/include)
+
 #include "base64-inl.h"
 #include "base_object-inl.h"
 #include "env-inl.h"
 #include "memory_tracker-inl.h"
 #include "node_errors.h"
-#include "node_sockaddr-inl.h"  // NOLINT(build/include)
 #include "uv.h"
 
 #include <memory>
diff --git a/src/node_version.h b/src/node_version.h
index d3dff1c574..13a91e23ef 100644
--- a/src/node_version.h
+++ b/src/node_version.h
@@ -66,7 +66,9 @@
 
 #define NODE_VERSION_AT_LEAST(major, minor, patch)                             \
   (((major) < NODE_MAJOR_VERSION) ||                                           \
+  /* NOLINTNEXTLINE (whitespace/indent) */                                     \
    ((major) == NODE_MAJOR_VERSION && (minor) < NODE_MINOR_VERSION) ||          \
+  /* NOLINTNEXTLINE (whitespace/indent) */                                     \
    ((major) == NODE_MAJOR_VERSION && (minor) == NODE_MINOR_VERSION &&          \
     (patch) <= NODE_PATCH_VERSION))
 
diff --git a/src/node_worker.cc b/src/node_worker.cc
index 39694ee098..610f0da800 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -201,7 +201,7 @@ class WorkerThreadData {
 
       w_->platform_->AddIsolateFinishedCallback(
           isolate,
-          [](void* data) { *static_cast<bool*>(data) = true; },
+          [](void* data) { (*static_cast<bool*>(data)) = true; },
           &platform_finished);
 
       // The order of these calls is important; if the Isolate is first disposed
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
index 0b5af454aa..4645143159 100644
--- a/src/stream_base-inl.h
+++ b/src/stream_base-inl.h
@@ -72,7 +72,7 @@ void StreamResource::RemoveStreamListener(StreamListener* listener) {
   // Remove from the linked list.
   for (current = listener_, previous = nullptr;
        /* No loop condition because we want a crash if listener is not found */
-       ;
+       ;  // NOLINT(whitespace/semicolon)
        previous = current, current = current->previous_listener_) {
     CHECK_NOT_NULL(current);
     if (current == listener) {
diff --git a/test/addons/openssl-client-cert-engine/testengine.cc b/test/addons/openssl-client-cert-engine/testengine.cc
index 66da26c1eb..b27a9d20cf 100644
--- a/test/addons/openssl-client-cert-engine/testengine.cc
+++ b/test/addons/openssl-client-cert-engine/testengine.cc
@@ -50,7 +50,7 @@ int EngineLoadSSLClientCert(ENGINE* engine,
                             STACK_OF(X509_NAME) * ca_dn,
                             X509** ppcert,
                             EVP_PKEY** ppkey,
-                            STACK_OF(X509) * *pother,
+                            STACK_OF(X509)** pother,
                             UI_METHOD* ui_method,
                             void* callback_data) {
   if (ppcert != nullptr) {

tools,src: remove clang-format

26b3fa1

The clang-format tools generates code that is at odds with cpplint.py. Since we run cpplint.py on every commit and (apparently) almost never run clang-format, let's simplify things by removing clang-format.

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Apr 9, 2022

Trott added c++ Issues and PRs that require attention from people who are familiar with C++. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 9, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2022

targos requested changes Apr 9, 2022

View reviewed changes

Trott added a commit to Trott/io.js that referenced this pull request Apr 9, 2022

src,crypto: adjust file for formatter/linter

91a8d7c

Refs: nodejs#42665 (comment)

Trott added a commit to Trott/io.js that referenced this pull request Apr 9, 2022

src,crypto: adjust crypto_bio files for formatter/linter

d0b3853

Refs: nodejs#42665 (comment)

Trott added a commit to Trott/io.js that referenced this pull request Apr 9, 2022

src,crypto: adjust crypto_bio files for formatter/linter

1948149

Run clang-format on the files and provide needed additional comments for the linter. Refs: nodejs#42665 (comment)

Trott mentioned this pull request Apr 9, 2022

src,crypto: adjust crypto_bio files for formatter/linter #42668

Closed

Trott mentioned this pull request Apr 10, 2022

build: run clang-format on CI #42681

Merged

Trott closed this Apr 10, 2022

Trott deleted the clang-format branch April 10, 2022 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

tools,src: remove clang-format #42665

tools,src: remove clang-format #42665

Uh oh!

Trott commented Apr 9, 2022

Uh oh!

nodejs-github-bot commented Apr 9, 2022

Uh oh!

targos left a comment

Uh oh!

RaisinTen commented Apr 9, 2022

Uh oh!

Trott commented Apr 9, 2022

Uh oh!

Trott commented Apr 9, 2022 •

edited

Loading

Uh oh!

Trott commented Apr 9, 2022

Uh oh!

RaisinTen commented Apr 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

tools,src: remove clang-format #42665

tools,src: remove clang-format #42665

Uh oh!

Conversation

Trott commented Apr 9, 2022

Uh oh!

nodejs-github-bot commented Apr 9, 2022

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented Apr 9, 2022

Uh oh!

Trott commented Apr 9, 2022

Uh oh!

Trott commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Apr 9, 2022

Uh oh!

RaisinTen commented Apr 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Trott commented Apr 9, 2022 •

edited

Loading