Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: simplify SFINAE of ToStringHelper::BaseConvert #44306

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

F3n67u
Copy link
Member

@F3n67u F3n67u commented Aug 20, 2022

Simplify SFINAE of ToStringHelper::BaseConvert using technique from https://www.fluentcpp.com/2018/05/18/make-sfinae-pretty-2-hidden-beauty-sfinae/

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 20, 2022
@F3n67u F3n67u changed the title src: simplify enable_if logic of ToStringHelper::BaseConvert src: simplify SFINAE of ToStringHelper::BaseConvert Aug 20, 2022
@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2022
@F3n67u F3n67u added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 1, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 1, 2022
@nodejs-github-bot nodejs-github-bot merged commit f36813c into nodejs:main Sep 1, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f36813c

@F3n67u F3n67u deleted the simplify_enable_if branch September 1, 2022 06:38
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
PR-URL: #44306
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
PR-URL: #44306
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
PR-URL: #44306
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44306
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@juanarbol
Copy link
Member

For some reason, this is not compiling in the v16.x branch. It may be related to a cpp version or something.

esult -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++14 -MMD -MF /home/juanarbol/GitHub/node/out/Release/.deps//home/juanarbol/GitHub/node/out/Release/obj.target/libnode/src/module_wrap.o.d.raw   -c
In file included from ../src/js_stream.cc:5:
In file included from ../src/node_errors.h:6:
../src/debug_utils-inl.h:32:46: error: no template named 'is_integral_v' in namespace 'std'; did you mean 'is_integral'?
            typename = std::enable_if_t<std::is_integral_v<T>>>
                                        ~~~~~^~~~~~~~~~~~~
                                             is_integral
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/type_traits:392:12: note: 'is_integral' declared here
    struct is_integral
           ^
In file included from ../src/js_stream.cc:5:
In file included from ../src/node_errors.h:6:
../src/debug_utils-inl.h:32:41: error: template argument for non-type template parameter must be an expression
            typename = std::enable_if_t<std::is_integral_v<T>>>

And the complaints grow and grow.

@RaisinTen
Copy link
Contributor

is_integral_v is C++17 only, so we would need to replace that with is_integral<T>::value if we want to backport (probably fine if we don't backport this too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants