fix gcc 8 function warning

[mmomtchev]

How about this fix for #807 ? It is very clean, but needs testing on different compilers

[agnat]

Hm, this is odd. I still had a non-good feeling about this and I tried to reproduce this kind of "variable argument function". I can not get it to work. Could you take a look?

extern "C" {
  struct req { int dummy; };
  typedef void (*old_cb)(req * r);
  typedef void (*new_cb)(req * r, int status);

  void old_api(old_cb) {};
  void new_api(new_cb) {};
};

inline void complete(req * r, int status = 0) {}

int main(void) {
  old_api(complete);
  new_api(complete);
}

This is the same thing, right? Well, it does not compile:

$ c++ --std c++17 ttt.cpp -o ttt
ttt.cpp:14:3: error: no matching function for call to 'old_api'
  old_api(complete);
  ^~~~~~~
ttt.cpp:8:6: note: candidate function not viable: no known conversion from 'void (req *, int)' to 'old_cb'
      (aka 'void (*)(req *)') for 1st argument
void old_api(old_cb){};
     ^
1 error generated.

This is on OSX using clang. Am I missing something? According to your research this should work, right?

[kkoopa]

I think this is the relevant part:

§12.4.1.3 https://eel.is/c++draft/over.match#general-3

If a best viable function exists and is unique, overload resolution succeeds and produces it as the result. Otherwise overload resolution fails and the invocation is ill-formed.

It is okay to declare, but must produce an error with a correct compiler if ambiguous.

On October 9, 2020 8:21:49 PM GMT+03:00, David Siegel notifications@github.com wrote:

Hm, this is odd. I still had a non-good feeling about this and I tried to reproduce this kind of "variable argument function". I can not get it to work. Could you take a look?

extern "C" {
 struct req { int dummy; };
 typedef void (*old_cb)(req * r);
 typedef void (*new_cb)(req * r, int status);

 void old_api(old_cb) {};
 void new_api(new_cb) {};
};

inline void complete(req * r, int status = 0) {}

int main(void) {
 old_api(complete);
 new_api(complete);
}

This is the same thing, right? Well, it does not compile:

$ c++ --std c++17 ttt.cpp -o ttt
ttt.cpp:14:3: error: no matching function for call to 'old_api'
 old_api(complete);
 ^~~~~~~
ttt.cpp:8:6: note: candidate function not viable: no known conversion
from 'void (req *, int)' to 'old_cb'
     (aka 'void (*)(req *)') for 1st argument
void old_api(old_cb){};
    ^
1 error generated.

This is on OSX using clang. Am I missing something? According to your research this should work, right?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/nodejs/nan/pull/899#issuecomment-706305100

[agnat]

Thanks! So, I did recall correctly in this this comment. It is ambiguous. Ok, meta function it is ... or #ifdef's. 😜

Why we don't hit this in testing is another matter. We have tests for async workers, right? 🤔

[kkoopa]

Have not checked the test suite, but assuming it is tested: g++ is sloppy (at least by default it allows language extensions and such). It could also be that the compiler versions in use are somewhat old.

On October 9, 2020 8:58:28 PM GMT+03:00, David Siegel notifications@github.com wrote:

Thanks! So, I did recall correctly in this this comment. It is ambiguous. Ok, meta function it is ... or #ifdef's. 😜>

Why we don't hit this in testing is another matter. We have tests for async workers, right? 🤔>

-- > You are receiving this because you were mentioned.> Reply to this email directly or view it on GitHub:> https://github.com/nodejs/nan/pull/899#issuecomment-706323681

[agnat]

Yes, but this PR should have tripped the osx build, which uses clang, no?

[kkoopa]

IIRC, it is not really clang, but Apple's hack. Who knows what they have done to it. Could also be that it is a somewhat old version which is non-compliant.

I like the metafunction myself, since it is definitely compliant (based on visual inspection).

On October 9, 2020 9:06:35 PM GMT+03:00, David Siegel notifications@github.com wrote:

Yes, but this PR should have tripped the osx build, which uses clang, no?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/nodejs/nan/pull/899#issuecomment-706327436

[addaleax]

This PR currently creates only a two-argument version of the function. The fact that there is a default value for status only affects direct calls to the function, not calls made to it through the function pointer, because as a function pointer it always has the type void (*)(uv_work_t*, int) now.

That is, if you want to have a function that can be used as a function pointer which receives either 1 or 2 arguments, you need to use overloads instead of default arguments:

// Variant 1: Previous code, only 1-argument version, needs cast
void AsyncExecuteComplete (uv_work_t* req) { ... }

// Variant 2: Previous code, only 2-argument version, will not work as a 1-argument version (!)
// `= 0` can be dropped, it doesn’t change anything in this context
void AsyncExecuteComplete (uv_work_t* req, int status = 0) { ... }

// Variant 3: Both variants, one implicitly calls the other, works everywhere
void AsyncExecuteComplete (uv_work_t* req) { ... }
void AsyncExecuteComplete (uv_work_t* req, int status) {
  AsyncExecuteComplete(req);
}

With Variant 3, the compiler should automatically pick the correct one to pass to uv_queue_work().

[kkoopa]

Thank you, this makes sense and fits with a blog post I was just reading: https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/

TLDR: Never use default function parameters. It is not worth the trouble.

On October 9, 2020 9:21:00 PM GMT+03:00, Anna Henningsen notifications@github.com wrote:

This PR currently creates only a two-argument version of the function. The fact that there is a default value for status only affects direct calls to the function, not calls made to it through the function pointer, because as a function pointer it always has the type void (*)(uv_work_t*, int) now.>

That is, if you want to have a function that can be used as a function pointer which receives either 1 or 2 arguments, you need to use overloads instead of default arguments:>

// Variant 1: Previous code, only 1-argument version, needs case>
void AsyncExecuteComplete (uv_work_t* req) { ... }>

// Variant 2: Previous code, only 2-argument version, will not work as
a 1-argument version (!)>
// `= 0` can be dropped, it doesn’t change anything in this context>
void AsyncExecuteComplete (uv_work_t* req, int status = 0) { ... }>

// Variant 3: Both variants, one implicitly calls the other, works
everywhere>
void AsyncExecuteComplete (uv_work_t* req) { ... }>
void AsyncExecuteComplete (uv_work_t* req, int status) {>
AsyncExecuteComplete(req);>
}>
```>

With Variant 3, the compiler should automatically pick the correct one
to pass to `uv_queue_work()`.>

-- >
You are receiving this because you were mentioned.>
Reply to this email directly or view it on GitHub:>
https://github.com/nodejs/nan/pull/899#issuecomment-706333871

[agnat]

@addaleax is right. Variant 3 works:

#include <iostream>
extern "C" {
  struct req { char dummy; };
  typedef void (*old_cb)(req * r);
  typedef void (*new_cb)(req * r, int status);

  void old_api(old_cb f){ f(0); };
  void new_api(new_cb f){ f(0, 0); };

};

inline void complete(req * r) { std::cerr << "0\n"; }
inline void complete(req * r, int status) { std::cerr << "1\n"; }

int main(void) {
  old_api(complete);
  new_api(complete);
}

I like it. Overloads don't scale, but we only need two. And libuv is not exactly a moving target.

[agnat]

Exactly! Way too subtle. 😜

[kkoopa]

Thank you all for finally getting this addressed. Since it is weekend, I will make a release during next week.