Skip to content

src: refactor macro to std::min in node_buffer.cc#25919

Closed
gengjiawen wants to merge 1 commit intonodejs:masterfrom
gengjiawen:fix_macro_define
Closed

src: refactor macro to std::min in node_buffer.cc#25919
gengjiawen wants to merge 1 commit intonodejs:masterfrom
gengjiawen:fix_macro_define

Conversation

@gengjiawen
Copy link
Copy Markdown
Member

@gengjiawen gengjiawen commented Feb 4, 2019

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

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 4, 2019
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Feb 4, 2019

We use std::min elsewhere in our code, maybe it’s best to just replace the macro usage with that?

@gengjiawen
Copy link
Copy Markdown
Member Author

gengjiawen commented Feb 4, 2019

@addaleax I will give it a try.

@gengjiawen gengjiawen force-pushed the fix_macro_define branch 2 times, most recently from 04407e9 to 57b2d35 Compare February 4, 2019 10:35
@gengjiawen gengjiawen changed the title src: avoid macro re-define in node_buffer.cc src: refactor macro to std::min in node_buffer.cc Feb 4, 2019
bnoordhuis
bnoordhuis approved these changes Feb 4, 2019
View reviewed changes
@refack
Copy link
Copy Markdown
Contributor

refack commented Feb 4, 2019

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

ChALkeR
ChALkeR reviewed Feb 7, 2019
View reviewed changes
@gengjiawen
Copy link
Copy Markdown
Member Author

gengjiawen commented Feb 9, 2019

Since we can't now format the whole file, I run into some issue on windows wsl:

Formatting C++ diff from HEAD..
  File "tools/clang-format/node_modules/.bin/git-clang-format", line 2
    basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
            ^
SyntaxError: invalid syntax
Makefile:1254: recipe for target 'format-cpp' failed
make: *** [format-cpp] Error 1

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Feb 9, 2019

@gengjiawen That is npm trying to work around the lack of symlinks on Windows…

If you look at the definition of format-cpp in the Makefile, I think you might be able to resolve this by replacing .bin/git-clang-format and .bin/clang-format with their respective targets (clang-format/bin/git-clang-format and clang-format/index.js)?

@gengjiawen
Copy link
Copy Markdown
Member Author

gengjiawen commented Feb 9, 2019 •
edited
Loading

@addaleax Thanks. I already clang-format the whole file. revert unrelated change. Easier when I get help from CLion :)

@gengjiawen
Copy link
Copy Markdown
Member Author

gengjiawen commented Feb 9, 2019

In the long run we should will try to make it more easier as described in #25908. I will try to invest more on this recently.

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Feb 9, 2019

Landed in 2fa6376

@addaleax addaleax closed this Feb 9, 2019
addaleax pushed a commit that referenced this pull request Feb 9, 2019
@gengjiawen @addaleax
src: refactor macro to std::min in node_buffer.cc
2fa6376 
PR-URL: #25919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
targos pushed a commit that referenced this pull request Feb 10, 2019
@gengjiawen @targos
src: refactor macro to std::min in node_buffer.cc
779a577 
PR-URL: #25919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Merged
@beevelop beevelop mentioned this pull request Feb 15, 2019
Closed
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@gengjiawen @addaleax @refack @bnoordhuis @ChALkeR @jasnell @lpinca @cjihrig @nodejs-github-bot