fix gcc 8 function warning #899

mmomtchev · 2020-10-08T09:15:54Z

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

agnat

Hi @mmomtchev,
thanks for looking into this. I agree with @kkoopa's comment from the other thread. I'd just use the cast. Unless I'm missing something?

Now that I think about it, I'm not even sure why this works... if it works... very sneaky.

agnat · 2020-10-08T21:43:50Z

nan.h

@@ -2280,7 +2280,7 @@ inline void AsyncExecute (uv_work_t* req) {
  worker->Execute();
 }

-inline void AsyncExecuteComplete (uv_work_t* req) {
+inline void AsyncExecuteComplete(uv_work_t* req, int status = 0) {


Hehe, to be honest, I think it is a bit too subtle. But it is kind of pretty ... in a sneaky way. Can we drop the default value and just use a hard cast a couple of lines below?

reinterpret_cast<(void (*)(void))>(AsyncExecuteComplete)

... or similar?

Sure we can, but for me this is the root cause of the problem - AsyncExecuteComplete can be called with 1 or with 2 arguments - not depending on the circumstances like most optional arguments, but depending on the libuv version
So this is, in a way, a variable argument function 😃

Hehe, yes. I do understand the idea. But it's not really. I'm a bit rusty, but IIRC a function with a default argument value behaves exactly like an overloaded function. There are two valid signatures. The compiler has to choose at compile-time. So, how is this not ambiguous? Anyway, since your solution does not get rid of the hard reinterpret_cast<>() either, I don't see a real advantage of doing it this way.

It's not that I'm totally opposed here, but to merge this I need a thorough explanation how and why this works. At the very least it needs a source code comment. In two weeks nobody knows why this thing has a default argument. You have to admit, considering that it just side-steps a warning it is rather obscure...

Here is a meta function that avoids the warning and gets rid of the cast. I didn't really test it, but this should do it:

inline void AsyncExecuteComplete (uv_work_t* req) { AsyncWorker* worker = static_cast<AsyncWorker*>(req->data); worker->WorkComplete(); worker->Destroy(); } template <typename CB> class UVAfterWorkAdapter; template <> class UVAfterWorkAdapter<void (*)(uv_work_t *)> { void invoke(uv_work_t * req) { AsyncExecuteComplete(req); } }; template <> class UVAfterWorkAdapter<void (*)(uv_work_t *, int)> { void invoke(uv_work_t * req, int status) { AsyncExecuteComplete(req); } }; inline void AsyncQueueWorker (AsyncWorker* worker) { uv_queue_work( GetCurrentEventLoop() , &worker->request , AsyncExecute , UVAfterWorkAdapter<uv_after_work_cb>::invoke ); }

Very good point, the cast is not needed with the optional parameter at all
No need for an intermediate template

Ah, nice! Yes, that I can understand. How did I miss that? 🤔 Anyway... could you add a little comment on AsyncExecuteComplete explaining the default argument?

agnat · 2020-10-09T17:21:31Z

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 · 2020-10-09T17:40:37Z

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 ***@***.***> 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: #899 (comment)

agnat · 2020-10-09T17:58:12Z

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 · 2020-10-09T18:05:28Z

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 ***@***.***> wrote: Thanks! So, I did recall correctly in this [this comment](#899 (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:> #899 (comment)

agnat · 2020-10-09T18:06:18Z

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

kkoopa · 2020-10-09T18:14:20Z

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 ***@***.***> 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: #899 (comment)

addaleax · 2020-10-09T18:20:45Z

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 · 2020-10-09T18:30:54Z

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 ***@***.***> 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:> > ```c++> // 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:> #899 (comment)

agnat · 2020-10-09T18:33:09Z

@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 · 2020-10-09T18:34:12Z

Exactly! Way too subtle. 😜

agnat

Almost there. One bug, one performance nit...

agnat · 2020-10-09T20:45:47Z

nan.h

  AsyncWorker* worker = static_cast<AsyncWorker*>(req->data);
  worker->WorkComplete();
  worker->Destroy();
 }
+void AsyncExecuteComplete (uv_work_t* req, int status) {
+  AsyncExecute(req);


Should be AsyncExecuteComplete(req), right?

agnat · 2020-10-09T20:52:31Z

nan.h

@@ -2284,11 +2284,14 @@ inline void AsyncExecute (uv_work_t* req) {
 * 2 arguments since node-v0.9.4
 * https://github.com/libuv/libuv/commit/92fb84b751e18f032c02609467f44bfe927b80c5
 */
-inline void AsyncExecuteComplete(uv_work_t* req, int status = 0) {


Better keep it inline. It doesn't matter for the old signature, but it does for the new one with status below.

doesn't build with g++-8
I wonder if you can take the address of an overloaded function by resolving the fingerprint? I think this works only for direct calls too?

https://en.cppreference.com/w/cpp/language/overloaded_address
yes, it should work

This is in a header file, so not keeping it inline has the potential to lead to linking errors if an addon happens to include nan twice (I know this wouldn’t be a common scenario, but it is a realistic one)

I don’t see why keeping it inline wouldn’t work, though, that surprises me – this should generally be possible with GCC 8:

https://godbolt.org/z/qx9szb

It is not about the inline:

/home/mmom/src/node-gdal-async/build/../node_modules/nan/nan.h:2292: multiple definition of `Nan::AsyncExecuteComplete(uv_work_s*, int)'; Debug/obj.target/gdal/src/utils/typed_array.o:/home/mmom/src/node-gdal-async/build/../node_modules/nan/nan.h:2292: first defined here

I get a weird error for multiple definitions, both of them on the same line 😄

Both need to be inline.
This works fine with
Apple clang version 12.0.0 (clang-1200.0.32.2)

diff --git a/nan.h b/nan.h index 580d3b5..6135f73 100644 --- a/nan.h +++ b/nan.h @@ -2284,12 +2284,12 @@ inline void AsyncExecute (uv_work_t* req) { * 2 arguments since node-v0.9.4 * https://github.com/libuv/libuv/commit/92fb84b751e18f032c02609467f44bfe927b80c5 */ -void AsyncExecuteComplete(uv_work_t *req) { +inline void AsyncExecuteComplete(uv_work_t *req) { AsyncWorker* worker = static_cast<AsyncWorker*>(req->data); worker->WorkComplete(); worker->Destroy(); } -void AsyncExecuteComplete (uv_work_t* req, int status) { +inline void AsyncExecuteComplete (uv_work_t* req, int status) { AsyncExecuteComplete(req); }

does anyone understand why the functions need to be inline?
@agnat's example works without it

The multifile test successfully detects the aforementioned multiple definition problem at the link stage. As @addaleax mentioned, this is a header library. In a header, ordinary, free functions may only be defined if marked inline, which, in C++ (at least from 98 onwards), has nothing to do with actual inlining of code (that is an implementation optional detail of the compiler, which a modern compiler mostly ignores. In g++, I think it might affect the inlinability-score slightly) . What the keyword actually means is that multiple definitions are allowed, with more leeway since C++17.

It’s the “realistic scenario” I talked above – if an addon includes nan.h twice, i.e. from two different files, then both of those addons will contain definitions for these functions. For inline functions that’s allowed, for regular functions not (because one can’t know in advance whether inline functions will have an explicit definition after compiling or not, because they might have been, well, inlined 🙂 ).

As an addendum, the C++17 thing I mentioned applies to inline variables, I misremembered that it had anything to do with functions. Note that for non-free functions (member functions), inline is implicit if entirely defined in the class declaration. If the definition is outside the class declaration, inline is not implicit.

agnat

LGTM.

kkoopa · 2020-10-09T22:46:46Z

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

fix gcc 8 function warning

3fb705b

agnat reviewed Oct 8, 2020

View reviewed changes

Momtchil Momtchev added 2 commits October 9, 2020 09:55

the cast is not needed with the opt argument

d304705

explain the weird optionnal argument

46a3ab8

adopt the overload proposal

62f5b11

agnat reviewed Oct 9, 2020

View reviewed changes

Momtchil Momtchev added 2 commits October 9, 2020 23:07

fix typo

80f20dc

inline both functions

69a8ca6

agnat approved these changes Oct 9, 2020

View reviewed changes

kkoopa merged commit 35f0fab into nodejs:master Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix gcc 8 function warning #899

fix gcc 8 function warning #899

mmomtchev commented Oct 8, 2020

agnat left a comment

agnat Oct 8, 2020

mmomtchev Oct 8, 2020

agnat Oct 8, 2020

agnat Oct 9, 2020

mmomtchev Oct 9, 2020

agnat Oct 9, 2020

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

addaleax commented Oct 9, 2020 •

edited

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

agnat commented Oct 9, 2020

agnat left a comment

agnat Oct 9, 2020

agnat Oct 9, 2020

mmomtchev Oct 9, 2020

mmomtchev Oct 9, 2020

addaleax Oct 9, 2020 •

edited

mmomtchev Oct 9, 2020

kkoopa Oct 9, 2020

mmomtchev Oct 9, 2020 •

edited

kkoopa Oct 9, 2020

addaleax Oct 9, 2020

kkoopa Oct 9, 2020

agnat left a comment

kkoopa commented Oct 9, 2020

fix gcc 8 function warning #899

fix gcc 8 function warning #899

Conversation

mmomtchev commented Oct 8, 2020

agnat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

kkoopa commented Oct 9, 2020 via email

addaleax commented Oct 9, 2020 • edited

kkoopa commented Oct 9, 2020 via email

agnat commented Oct 9, 2020

agnat commented Oct 9, 2020

agnat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Oct 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmomtchev Oct 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnat left a comment

Choose a reason for hiding this comment

kkoopa commented Oct 9, 2020

addaleax commented Oct 9, 2020 •

edited

addaleax Oct 9, 2020 •

edited

mmomtchev Oct 9, 2020 •

edited