★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/23793Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: refactor out some warnings from `FillStatsArray` and `node_file.h` #23793

Merged
merged 3 commits into from Oct 24, 2018

Conversation

Reviewers

@addaleax addaleax

@joyeecheung joyeecheung

Assignees
No one assigned
Labels
C++ backport-requested-v10.x lib / src
Projects
  
Don't land (for now) in v11.x
Milestone
No milestone
6 participants
@refack @nodejs-github-bot @addaleax @targos @MylesBorins @joyeecheung
@refack
Copy link
Member

commented Oct 21, 2018 •
edited

#### adds a the Guideline Support Library to /deps/:

src: refactor out warnings from FillStatsArray

  • move FillStatsArray to node_file.h minimize number of template instantions
  • change inline to constexpr
  • change MACRO consts to constexpr
  • correct polymorphic conversion from uv_timespec_t
  • DRY FillGlobalStatsArray
  • change FillGlobalStatsArray signature to have one less default arg
  • reduce some includes

src: clean clang-tidy errors in node_file.h

  • fix bug with non virtual dtor
  • delete move constructors
  • default initialize all members
  • discard unneeded return values
  • const some possibles
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@refack refack requested review from addaleax, joyeecheung and gireeshpunathil Oct 21, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the meta label Oct 21, 2018

@refack refack self-assigned this Oct 21, 2018

@refack refack added C++ lib / src labels Oct 21, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18014/
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018

@addaleax, I believe MallocedBuffer<T> can be our implementation of dyn_array<T>. It seems like that one is an elusive to implement genetically, and it's missing from the current implementation of the GSL.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Oct 21, 2018

This seems like a huge change for something that could be achieved through adding a couple of static_casts…
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018 •
edited

This seems like a huge change for something that could be achieved through adding a couple of static_casts…

It's a refactor that makes sense. Besides eliminating the warnings, we take out a template was was instantiated twice in every translation unit it was included, while it's actually used in to. Plus reducing node_internals.h which is kinda big.

And the GSL is just a big bunch of free win.

P.S. the second commit fe6cd36 is not that big...
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Oct 21, 2018

I’m totally on board with moving this out of node_internals.h, sure… just, this mixes a pretty decent-size dependency in for something where we don’t really need it…
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018

I’m totally on board with moving this out of node_internals.h, sure… just, this mixes a pretty decent-size dependency in for something where we don’t really need it…

I can add it in a separate PR, I thought this would be a nice POC...

But it just found a bug on Windows:

fields->SetValue(offset + 7, gsl::narrow<NativeT>(s->st_ino))

Where NativeT == double and s->st_ino == 31525197391720945 -> (double)s->st_ino == 31525197391720944

@refack refack force-pushed the refack:GSL branch from fe6cd36 to 86852c7 Oct 21, 2018

@joyeecheung

joyeecheung reviewed Oct 21, 2018
View changes

Copy link
Member

left a comment

Can you use static_cast in this PR, and leave the gsl part to another one as a follow up?

Anyway I am happy to see this out of node_internals.h!👍

@refack refack force-pushed the refack:GSL branch from 86852c7 to ee63a3b Oct 21, 2018

@refack refack changed the title src: refactor out warnings from `FillStatsArray` src: refactor out some warnings from `FillStatsArray` and `node_file.h` Oct 21, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018 •
edited

Took out the GSL, but added a second commit with few bug fixes found with clang-tidy.
Simply instantiating FillStatsArray only twice reduces number of warning by order of magnitude (from ~900 to ~90)
PTAL.

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

@refack refack force-pushed the refack:GSL branch from ee63a3b to 4194693 Oct 21, 2018

@refack refack removed the meta label Oct 21, 2018

@addaleax

addaleax reviewed Oct 21, 2018
View changes

src/node_file.cc Outdated
@@ -2319,6 +2319,8 @@ void Initialize(Local<Object> target,
use_promises_symbol).FromJust();
}



This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

Unrelated whitespace change

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

Ack
src/node_file.h
@@ -30,7 +30,7 @@ class FSContinuationData : public MemoryRetainer {

uv_fs_t* req;
int mode;
std::vector<std::string> paths;
std::vector<std::string> paths{};

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

Unrelated?

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

"ctor does not initialize paths"

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

That seems like something we should address on the compiler side, rather than starting to fix up all of our code… it’s not like something bad is happening here

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

The compiler can't tell if we want the default constructor or we forgot to initialize.
I'd rather be explicit, either here or in all the constructors.
src/node_file.h Outdated
@@ -187,11 +263,12 @@ class FSReqPromise : public FSReqBase {
object()->Get(env()->context(),
env()->promise_string()).ToLocalChecked();
Local<Promise::Resolver> resolver = val.As<Promise::Resolver>();
resolver->Resolve(env()->context(), value).FromJust();
static_cast<void>(resolver->Resolve(env()->context(), value).FromJust());

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

We use USE(…) to serve this purpose (inspired by the same function in V8) rather than casting to void

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

IMHO it is clearer...

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

USE isn’t a macro either, fwiw, just named that way because it used to be one in V8 I think.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

Ack
src/node_file.h
private:
bool finished_ = false;
AliasedBuffer<NativeT, V8T> stats_field_array_;
DISALLOW_COPY_AND_ASSIGN(FSReqPromise);

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

Can you explain these changes? If we are getting warnings here, we might want to revert 97f1e94 instead?

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member
  1. It's a MACRO
  2. It in private
  3. Does not delete the moves

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Oct 23, 2018 •
edited

Member

I think it's fine to use a macro if it reduces repetition, and it's also easier to search..about deleting move constructor&operator, why not just add another macro? (oops, I know)

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

The move variants are implicitly deleted, according to #23092

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

In this case I'm in favor being explicit over less-code-via-macros.

As for the move semantics, they are not generated, but are left undefined so it's a clang-tidy error, since the compiler can't say what we ment (not implement or forgot to implement).

#23092 is sort of Ok, since the MACRO is named DISALLOW_COPY_AND_ASSIGN not DISALLOW_COPY_AND_ASSIGN_AND_MOVE

P.S. C++20 might solve this with metaclasses
src/node_file.h Outdated
constexpr void FillStatsArrays(AliasedBuffer<NativeT, V8T>* fields,
const uv_stat_t* s,
const uv_stat_t* s2 = nullptr) {
if (s2) {

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

We try to use explicit comparisons for pointers, i.e. if (s2 != nullptr)

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

I thought I saw the opposite suggestions in a review this week 🤔
Anyway our styleguide has ES.87: Don’t add redundant == or != to conditions

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 21, 2018

Member

@refack That conflicts 100 % with our current style, i.e. it’s an omission in our style guide.

I’ve opened #23805 to document this.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 21, 2018

Author Member

Ack.

@refack refack force-pushed the refack:GSL branch from edc9503 to a8e1f04 Oct 21, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018

Reverted changes to grow_async_ids_stack in favor of #23808
https://ci.nodejs.org/job/node-test-pull-request/18032/
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 21, 2018

Fixed bad static_cast (those are not as simple as you'd expect);
CI: https://ci.nodejs.org/job/node-test-commit/22525/
@joyeecheung

joyeecheung reviewed Oct 23, 2018
View changes

src/node_file.h Outdated
inline Local<Value> FillGlobalStatsArray(Environment* env,
bool use_bigint,
const uv_stat_t* s,
const uv_stat_t* s2 = nullptr) {

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Oct 23, 2018

Member

Is there a reason the second call is encapsulated in node_file.h? It should be easier to understand if the outmost caller just call it twice? (as that's only specific to watch methods, and only the JS part for those methods will make use of the additional fields)

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

It made for a nice signature, also completely abstracted that we use the same AliasedArray for both. But after I refactored it, I got rid of a function, so it's also nice. PTAL.
@addaleax

addaleax approved these changes Oct 23, 2018
View changes

src/node_file.h
@@ -30,7 +30,7 @@ class FSContinuationData : public MemoryRetainer {

uv_fs_t* req;
int mode;
std::vector<std::string> paths;
std::vector<std::string> paths{};

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

That seems like something we should address on the compiler side, rather than starting to fix up all of our code… it’s not like something bad is happening here
src/node_file.h Outdated
bool second = false) {
ptrdiff_t offset = second ? kFsStatsFieldsNumber : 0;
if (use_bigint) {
const auto arr = env->fs_stats_field_bigint_array();

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

I understand the desire to not spell out the full type here, but could you make it const auto*? That way it’s obvious that no copy operations take place here, not knowing the type of arr

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018 •
edited

Author Member

I wanted to, but the compiler was being strange, I'll check again.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018 •
edited

Author Member 
error C2664: 'void node::fs::FillStatsArray<uint64_t,v8::BigUint64Array>(node::AliasedBuffer<uint64_t,v8::BigUint64Array> *,const uv_stat_t *,size_t)': 
cannot convert argument 1 from 'const node::AliasedBuffer<uint64_t,v8::BigUint64Array> *' to 'node::AliasedBuffer<uint64_t,v8::BigUint64Array> *' [D:\code\node\node_lib.vcxproj]

It's a covariance const thing.
I could define a using for the two variants...

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

It’s a const thing – auto*, not const auto* is what I should have written.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

Ack. It's a const auto * vs auto* const.
Done.
src/node_file.h Outdated
FillStatsArray(arr, s, offset);
return arr->GetJSArray();
} else {
const auto arr = env->fs_stats_field_array();

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

ditto

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

Done
@addaleax

addaleax approved these changes Oct 23, 2018
View changes

src/node_file.h Outdated
@@ -319,7 +411,7 @@ class FileHandle : public AsyncWrap, public StreamBase {
ref_.Reset(env->isolate(), ref);
}

~CloseReq() {
virtual ~CloseReq() {

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Oct 23, 2018

Member

Wouldn’t override do the job here? It’s what you used in other PRs, I think.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Author Member

Yep.
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 23, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/18061/
@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 23, 2018 •
edited

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

@refack refack merged commit 205b667 into nodejs:master Oct 24, 2018
1 check was pending

1 check was pending

node-test-commit-linuxone running tests
Details

@refack refack deleted the refack:GSL branch Oct 24, 2018

@refack refack removed their assignment Oct 24, 2018

targos added a commit that referenced this pull request Oct 26, 2018

@refack @targos
src: refactor FillStatsArray 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
6467fe0

targos added a commit that referenced this pull request Oct 26, 2018

@refack @targos
src: fix resource leak in node::fs::FileHandle 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
17ba595

targos added a commit that referenced this pull request Oct 26, 2018

@refack @targos
src: clean clang-tidy errors in node_file.h 
* explicitly delete move overloads
* default initialize all members
* explicitly discard unused return values
* const some possibles

PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
88ac489

@targos targos referenced this pull request Oct 27, 2018

Merged

Release proposal: v11.1.0 #23922

@targos targos added the dont-land-on-v11.x label Nov 1, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 1, 2018

Marking dont-land-on-v11.x until nodejs/build#1542 is fixed

@targos targos added this to Don't land (for now) in v11.x Nov 1, 2018

@targos targos removed the dont-land-on-v11.x label Nov 18, 2018

targos added a commit that referenced this pull request Nov 18, 2018

@refack @targos
src: refactor FillStatsArray 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
98443b5

targos added a commit that referenced this pull request Nov 18, 2018

@refack @targos
src: fix resource leak in node::fs::FileHandle 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
f3dbc70

targos added a commit that referenced this pull request Nov 18, 2018

@refack @targos
src: clean clang-tidy errors in node_file.h 
* explicitly delete move overloads
* default initialize all members
* explicitly discard unused return values
* const some possibles

PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
68c0a11

rvagg added a commit that referenced this pull request Nov 28, 2018

@refack @rvagg
src: refactor FillStatsArray 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
c0a9a83

rvagg added a commit that referenced this pull request Nov 28, 2018

@refack @rvagg
src: fix resource leak in node::fs::FileHandle 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
260d777

rvagg added a commit that referenced this pull request Nov 28, 2018

@refack @rvagg
src: clean clang-tidy errors in node_file.h 
* explicitly delete move overloads
* default initialize all members
* explicitly discard unused return values
* const some possibles

PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
74c0a97

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed

codebytere added a commit that referenced this pull request Jan 13, 2019

@refack @codebytere
src: refactor FillStatsArray 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
acf7e7d

codebytere added a commit that referenced this pull request Jan 13, 2019

@refack @codebytere
src: fix resource leak in node::fs::FileHandle 
PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
Loading status checks…
538589e

codebytere added a commit that referenced this pull request Jan 13, 2019

@refack @codebytere
src: clean clang-tidy errors in node_file.h 
* explicitly delete move overloads
* default initialize all members
* explicitly discard unused return values
* const some possibles

PR-URL: #23793
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
Loading status checks…
05581aa

@codebytere codebytere referenced this pull request Jan 15, 2019

Merged

v10.15.1 proposal #25346

@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 29, 2019

This PR seems to be creating unexpected failures on v10.x BSD

Can someone please backport?

https://ci.nodejs.org/job/node-test-commit-freebsd/23692/nodes=freebsd10-64/console

21:57:44 In file included from ../src/env.cc:6:
21:57:44 ../src/node_file.h:161:19: error: no return statement in constexpr function
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:168:18: error: no function template matches function template specialization 'ToNative'
21:57:44 constexpr double ToNative(uv_timespec_t ts) {
21:57:44                  ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:180:20: error: no function template matches function template specialization 'ToNative'
21:57:44 constexpr uint64_t ToNative(uv_timespec_t ts) {
21:57:44                    ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:215:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 10, ToNative<NativeT>(s->st_atim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:228:5: note: in instantiation of function template specialization 'node::fs::FillStatsArray<unsigned long, v8::BigUint64Array>' requested here
21:57:44     FillStatsArray(arr, s, offset);
21:57:44     ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:216:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 11, ToNative<NativeT>(s->st_mtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:217:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 12, ToNative<NativeT>(s->st_ctim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:218:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 13, ToNative<NativeT>(s->st_birthtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:215:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 10, ToNative<NativeT>(s->st_atim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:232:5: note: in instantiation of function template specialization 'node::fs::FillStatsArray<double, v8::Float64Array>' requested here
21:57:44     FillStatsArray(arr, s, offset);
21:57:44     ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:216:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 11, ToNative<NativeT>(s->st_mtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:217:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 12, ToNative<NativeT>(s->st_ctim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:218:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 13, ToNative<NativeT>(s->st_birthtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 11 errors generated.
21:57:44 gmake[2]: *** [node_lib.target.mk:316: /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/node_lib/src/env.o] Error 1
21:57:44 rm 25dc971a40d6cb85b6b7264b2590e351c5d97ef4.intermediate c72314b5849ba99a8583636d997488ad6da171e7.intermediate f7457ada113d3cc0ab78869adac42ba732a9fff5.intermediate
21:57:44 gmake[1]: *** [Makefile:99: node] Error 2
21:57:44 gmake: *** [Makefile:475: build-ci] Error 2
21:57:45 Build step 'Execute shell' marked build as failure
21:57:45 Performing Post build task...
21:57:45 Match found for : : True
21:57:45 Logical operation result is TRUE
21:57:45 Running script  : #/bin/bash

@MylesBorins MylesBorins added the backport-requested-v10.x label Jan 29, 2019

@refack refack referenced this pull request Feb 11, 2019

Closed

src: apply clang-tidy rule performance-unnecessary-value-param #26042

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.