node-addon-api icon indicating copy to clipboard operation
node-addon-api copied to clipboard

Published 20 hours ago • verified publisher icon nodejs

src: fix -Wconversion msg/C++17 UB from unknown_array_type

Open zbjornson opened this issue 3 years ago • 2 comments

Opening gambit to fix #1066. Only lightly tested (I'll fix the errors, no need to comment on those). Seeking feedback on the approach before adding tests.

Breaking change: this makes code like TypedArrayOf<bool>::New() fail at compile-time instead of run-time. The other changes I think are non-breaking.

zbjornson avatar Apr 13 '22 02:04 zbjornson

Breaking change: this makes code like TypedArrayOf::New() fail at compile-time instead of run-time

I'm not sure we want a breaking change if your earlier comment:

I don't think this is really an issue, but it would be nice to get rid of the warning.

is correct. Is there no way to fix the warning without breaking existing code?

mhdawson avatar Apr 13 '22 19:04 mhdawson

We looked at this in detail today. The thought was that the right approach might be to remove the concept of an unknown type completely.

In the contructor with no options, then just use a default type (maybe int8)

In the contructor that passes in an napi_value then do the lookup immediately versus defering.

We can't think of any way this should affect working existing code.

mhdawson avatar Jul 29 '22 15:07 mhdawson

@KevinEady is going to take a look at implementing the suggestion to remove concept of an unknown type completely.

mhdawson avatar Sep 16 '22 15:09 mhdawson

Closing as this will be fixed by https://github.com/nodejs/node-addon-api/pull/1209 instead.

mhdawson avatar Oct 14 '22 15:10 mhdawson