Adding CPPLinting paths src/tracing/ also to make file #14497

Jeyanthinath · 2017-07-26T09:41:07Z

Added one more CppLinting path in src/tracing in makefile

checks:

Make CppLint

TimothyGu · 2017-07-26T13:23:51Z

Makefile

@@ -896,6 +896,9 @@ CPPLINT_FILES = $(filter-out $(CPPLINT_EXCLUDE), $(wildcard \
 	src/*.c \
 	src/*.cc \
 	src/*.h \
+	src/tracing/.c \
+	src/tracing/.cc \
+	src/tracing/.h \


You are missing the wildcard character before the file extension (*.c).

Also, use src/*/*.c instead of src/tracing/*.c so that this is future-proof.

ok , sure I thought I want to keep now as a folder specific.

I will do the changes

XadillaX · 2017-07-27T12:59:32Z

Have you ever done CPPLint locally and passed after you modifying the configuration?

XadillaX

LGTM if CPPLint passed locally and CI is green.

TimothyGu

@XadillaX src/tracing/* files don't pass cpplint last time I checked.

Jeyanthinath · 2017-07-27T14:48:27Z

I will check one more time @TimothyGu

update : My- bad I did made the make test before saving the file, now liniting issues fixed, please do review

TimothyGu · 2017-07-31T03:12:59Z

src/tracing/node_trace_buffer.cc

+  uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
+           [](uv_handle_t* signal) {
+      NodeTraceBuffer* buffer =
+                  reinterpret_cast<NodeTraceBuffer*>(signal->data);


Four-space indentation.

TimothyGu · 2017-07-31T03:13:44Z

src/tracing/node_trace_buffer.h

@@ -75,7 +75,7 @@ class NodeTraceBuffer : public TraceBuffer {
  // Used to wait until async handles have been closed.
  ConditionVariable exit_cond_;
  std::unique_ptr<NodeTraceWriter> trace_writer_;
-  // TODO: Change std::atomic to something less contentious.
+  // TODO(misterpoe) : Change std::atomic to something less contentious.


As I've written in #14490 (comment), I'd put matthewloring instead. Also, remove the space between ) and :.

sorry, I used blame alone not dig upto PR

TimothyGu · 2017-07-31T03:14:09Z

src/tracing/node_trace_writer.cc

@@ -110,8 +110,8 @@ void NodeTraceWriter::FlushSignalCb(uv_async_t* signal) {
  trace_writer->FlushPrivate();
 }

-// TODO: Remove (is it necessary to change the API? Since because of WriteSuffix
-// it no longer matters whether it's true or false)
+// TODO(misterpoe) : Remove (is it necessary to change the API?


TimothyGu · 2017-07-31T03:14:27Z

src/tracing/node_trace_writer.cc

+  uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
+           [](uv_handle_t* signal) {
+      NodeTraceWriter* trace_writer =
+                              static_cast<NodeTraceWriter*>(signal->data);


Ditto, use 4 spaces.

TimothyGu · 2017-07-31T03:15:54Z

src/tracing/trace_event.h

-static inline void SetTraceValue(v8::ConvertableToTraceFormat* convertable_value,
-                                    unsigned char* type, uint64_t* value) {
+static inline void SetTraceValue(
+                                v8::ConvertableToTraceFormat* convertable_value,


4 space indent

TimothyGu · 2017-07-31T09:29:09Z

src/tracing/node_trace_buffer.cc

+  uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
+           [](uv_handle_t* signal) {
+      NodeTraceBuffer* buffer =
+                            reinterpret_cast<NodeTraceBuffer*>(signal->data);


Still not changed.

TimothyGu · 2017-07-31T09:29:21Z

src/tracing/node_trace_writer.cc

+  uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
+           [](uv_handle_t* signal) {
+      NodeTraceWriter* trace_writer =
+                                static_cast<NodeTraceWriter*>(signal->data);


TimothyGu · 2017-07-31T09:29:29Z

src/tracing/trace_event.h

-static inline void SetTraceValue(v8::ConvertableToTraceFormat* convertable_value,
-                                    unsigned char* type, uint64_t* value) {
+static inline void SetTraceValue(
+                                v8::ConvertableToTraceFormat* convertable_value,


Jeyanthinath · 2017-08-01T06:25:53Z

I did the suggested changes, there is no style guide for indentation I affraid, so I used another properly indented one code to verify the changes @TimothyGu

TimothyGu · 2017-08-01T07:13:09Z

LGTM!

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

TimothyGu · 2017-08-01T10:38:53Z

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

refack · 2017-08-01T16:16:37Z

/cc @misterpoe @matthewloring

matthewloring

Is it possible to exclude specific files from the linter? src/tracing/trace_event.h and src/tracing/trace_event_common.h are slightly modified copies of V8 files and it would be good to avoid complicating their diffs. Overall the changes look fine. Thank you for cleaning this up.

matthewloring · 2017-08-01T16:45:58Z

src/tracing/node_trace_buffer.h

@@ -75,7 +75,7 @@ class NodeTraceBuffer : public TraceBuffer {
  // Used to wait until async handles have been closed.
  ConditionVariable exit_cond_;
  std::unique_ptr<NodeTraceWriter> trace_writer_;
-  // TODO: Change std::atomic to something less contentious.
+  // TODO(matthewloring): Change std::atomic to something less contentious.


Not required as part of this change but this TODO is no longer necessary. We thought this might cause issues when the initial PR was reviewed but it ended up not being problematic.

TimothyGu · 2017-08-02T04:45:53Z

@matthewloring Sure.

@Jeyanthinath You can exclude certain files by adding them to the CPPLINT_EXCLUDE variable, like so:

# These files were copied more or less verbatim from V8.
CPPLINT_EXCLUDE += src/tracing/trace_event.h src/tracing/trace_event_common.h

Search for CPPLINT_EXCLUDE in Makefile and put the lines above near the existing exclusions.

I believe this PR will be ready to be merged once you fix that and remove the TODO @matthewloring mentioned. 🥇

fix: nodejs#14490

TimothyGu · 2017-08-05T09:04:00Z

@Jeyanthinath Thanks for adding the exclusions, but you should also revert the changes from those files as @matthewloring requested :/

Jeyanthinath · 2017-08-05T13:13:16Z

src/tracing/node_trace_writer.cc

@@ -110,8 +110,8 @@ void NodeTraceWriter::FlushSignalCb(uv_async_t* signal) {
  trace_writer->FlushPrivate();
 }

-// TODO: Remove (is it necessary to change the API? Since because of WriteSuffix
-// it no longer matters whether it's true or false)
+// TODO(matthewloring): Remove (is it necessary to change the API?


@TimothyGu @matthewloring Is this is also needs to be removed ?

Nope, that one should stay in to remind me to clean up some of this. Thanks for checking!

Jeyanthinath · 2017-08-05T15:44:10Z

@TimothyGu I guess I have made the changes as you guys pointed out, removing on TODO from the file he mentioned, is there anything pending ?

refack

reaffirm

XadillaX · 2017-08-06T06:34:49Z

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

TimothyGu · 2017-08-07T02:15:59Z

@Jeyanthinath Sorry, I meant reverting the changes you've done to src/tracing/trace_event.h and src/tracing/trace_event_common.h.

Also for your next PR, please use our commit message guidelines, so for this specific PR you would write something like:

build: enable C++ linting for src/*/*

Fixes: https://github.com/nodejs/node/issues/14490

Don't worry about it for this specific PR though, I've landed this with those two mini-issues fixed in e88908d.

Fixes: nodejs#14490 PR-URL: nodejs#14497 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Refael Ackermann <refack@gmail.com>

Fixes: #14490 PR-URL: #14497 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-09-19T17:21:52Z

Most of the files included in this change do not appear to be on the v6.x branch. Please feel free to change the label and update if neccessary

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 26, 2017

TimothyGu suggested changes Jul 26, 2017

View reviewed changes

XadillaX approved these changes Jul 27, 2017

View reviewed changes

TimothyGu suggested changes Jul 27, 2017

View reviewed changes

Jeyanthinath force-pushed the jeyanthi#14490 branch from 4211973 to 29638c6 Compare July 28, 2017 18:50

TimothyGu reviewed Jul 31, 2017

View reviewed changes

TimothyGu self-requested a review July 31, 2017 05:15

Jeyanthinath force-pushed the jeyanthi#14490 branch from 29638c6 to 591a366 Compare July 31, 2017 05:46

TimothyGu reviewed Jul 31, 2017

View reviewed changes

TimothyGu self-assigned this Jul 31, 2017

TimothyGu approved these changes Aug 1, 2017

View reviewed changes

refack approved these changes Aug 1, 2017

View reviewed changes

matthewloring reviewed Aug 1, 2017

View reviewed changes

Jeyanthinath added 7 commits August 2, 2017 14:30

Adding CPPLinting paths src/tracing/ also to make file

2a283b7

fix: nodejs#14490

changing paths from src/tracing/ to src/* for future proofing

a851310

fix: nodejs#14490

Fixed linting issues in tracing folder

51882b7

fix: nodejs#14490

Added Correct todo and changed some tab space problems

abf6212

fix: nodejs#14490

Fixed Ident Problems

2b87197

fix: nodejs#14490

Fixed the space issue

db2c740

fix: nodejs#14490

Removed: ToDo and excluded some files from linting

76c5179

fix: nodejs#14490

Jeyanthinath force-pushed the jeyanthi#14490 branch from 6bbf635 to 76c5179 Compare August 2, 2017 09:07

Jeyanthinath commented Aug 5, 2017

View reviewed changes

refack approved these changes Aug 5, 2017

View reviewed changes

TimothyGu closed this Aug 7, 2017

addaleax pushed a commit that referenced this pull request Aug 7, 2017

build: enable C++ linting for src/*/*

c0f0c38

Fixes: #14490 PR-URL: #14497 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

Adding CPPLinting paths src/tracing/ also to make file #14497

Adding CPPLinting paths src/tracing/ also to make file #14497

Conversation

Jeyanthinath commented Jul 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jul 27, 2017 • edited

XadillaX left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Jeyanthinath commented Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jeyanthinath commented Aug 1, 2017 • edited

TimothyGu commented Aug 1, 2017

TimothyGu commented Aug 1, 2017

refack commented Aug 1, 2017

matthewloring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Aug 2, 2017

TimothyGu commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jeyanthinath commented Aug 5, 2017

refack left a comment

Choose a reason for hiding this comment

XadillaX commented Aug 6, 2017

TimothyGu commented Aug 7, 2017 • edited

MylesBorins commented Sep 19, 2017

Jeyanthinath commented Jul 26, 2017 •

edited

XadillaX commented Jul 27, 2017 •

edited

Jeyanthinath commented Jul 27, 2017 •

edited

Jeyanthinath commented Aug 1, 2017 •

edited

TimothyGu commented Aug 7, 2017 •

edited