src,doc: refactor to replace typedefs with usings#910
src,doc: refactor to replace typedefs with usings#910RaisinTen wants to merge 1 commit intonodejs:mainfrom
Conversation
|
What level of compiler support does this syntax require? |
|
@mhdawson all compilers support this except except Digital Mars C++. How do I fix the lint error? Running this doesn't change anything for me locally: ❯ npm run lint:fix
> node-addon-api@3.1.0 lint:fix
> git-clang-format '*.h', '*.cc'
no modified files to format |
|
One more question, is there anything that you can do with a typedef definition that you can't do with the using def ? |
|
@mhdawson no there isn't. |
The lint is comparing the local changes with the main branch. Please have a check on your local main branch and dev branch are up to date with the GitHub one. |
@legendecas Yes, they are up to date. Turns out, I tried to copy and paste the linter error log into a file and :/ |
|
PR to fix the error detection: #914 |
c51c971 to
77083ea
Compare
|
I could apply the patch from the logs of the failed style check run and now the style check passes too. However, it seems to have added some more changes than what I intended (like indenting some other parts of the code). PTAL. |
|
@nodejs/node-api let me know if you have any concerns about this one by Friday (or approve), otherwise I'll go ahead and land it. |
NickNaso
left a comment
There was a problem hiding this comment.
Some notes about the reasons that pushed me to approve this PR:
It's possible to define a new name for an existing type with both:
typedef(We call this declarationtypedefand the resulting nametypedef-name)using(We can call this decalrationalias desclaration)
The using alternative is more readable because you always have the type name on the left side of the =.
Unlike a typedef an alias declaration can be templated to provide a conveninet name for a family of types. This is available since C++11 and is called alias template.
|
Need to run on our CI to make sure all the older compilers support using before landing. |
|
CI: The test fails also with the |
|
@NickNaso thanks for running the CIs The failures on 11 seem unrelated to this PR as you mention, possibly because of something that would have been backported to 10.x but not 11. Checking the table in https://en.cppreference.com/w/cpp/compiler_support#cpp11 it also seems to confirm that all of the minimum levels for Node.js 10.x support using so I think this is good to land. |
PR-URL: #910 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
|
Landed as 71494a4 |
PR-URL: nodejs/node-addon-api#910 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
PR-URL: nodejs/node-addon-api#910 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
PR-URL: nodejs/node-addon-api#910 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
PR-URL: nodejs/node-addon-api#910 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
No description provided.