Skip to content

src: avoid name clash with upper scope in node_trace_buffer.cc - #25920

Closed
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:feature/redability_node_trace_buffer
Closed

src: avoid name clash with upper scope in node_trace_buffer.cc#25920
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:feature/redability_node_trace_buffer

Conversation

@gengjiawen

@gengjiawengengjiawen commented Feb 4, 2019

Copy link
Copy Markdown
Member
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 4, 2019
@addaleax

Copy link
Copy Markdown
Member

This conflicts with #25896.

I also find it confusing: These are the same objects, so referring to them by the same name makes sense, right?

@gengjiawen

Copy link
Copy Markdown
MemberAuthor

They are in the same function, so better with different name IMHO.

@gengjiawen
gengjiawenforce-pushed the feature/redability_node_trace_buffer branch from aff0acc to f25d583CompareFebruary 5, 2019 02:16
@gengjiawen
gengjiawenforce-pushed the feature/redability_node_trace_buffer branch from f25d583 to 6c18ed9CompareFebruary 5, 2019 02:19
@gengjiawen

Copy link
Copy Markdown
MemberAuthor

Current there is three buffer in this method, give those different name will make it more readable
image

@gengjiawen

Copy link
Copy Markdown
MemberAuthor

@addaleax give this another thought or leave it the old way ?

@addaleax

Copy link
Copy Markdown
Member

@gengjiawen Yeah, I’m still -1 on doing this. You can wait for others to chime in, if you want.

@gengjiawen
gengjiawen deleted the feature/redability_node_trace_buffer branch February 6, 2019 14:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@gengjiawen@addaleax@nodejs-github-bot