Skip to content

Commit

Permalink
Convert static_assert to enable_if in jsi::Value::Value(T&&) ctor (#3…
Browse files Browse the repository at this point in the history
…7520)

Summary:
Pull Request resolved: #37520

Results in better error messages, especially when trying to copy a Value.

With this change:

```
../API/hermes/hermes.cpp: In member function ‘facebook::jsi::Value facebook::hermes::HermesRuntimeImpl::valueFromHermesValue(hermes::vm::HermesValue)’:
../API/hermes/hermes.cpp:726:31: error: use of deleted function ‘facebook::jsi::Value::Value(const facebook::jsi::Value&)’
  726 |     auto copy = jsi::Value(val);
      |                               ^
In file included from ../API/hermes/hermes.h:18,
                 from ../API/hermes/hermes.cpp:8:
../API/jsi/jsi/jsi.h:938:18: note: ‘facebook::jsi::Value::Value(const facebook::jsi::Value&)’ is implicitly declared as deleted because ‘facebook::jsi::Value’ declares a move constructor or move assignment operator
  938 | class JSI_EXPORT Value {
      |                  ^~~~~
```

Before:

```
In file included from ../API/hermes/hermes.h:18,
                 from ../API/hermes/hermes.cpp:8:
../API/jsi/jsi/jsi.h: In instantiation of ‘facebook::jsi::Value::Value(T&&) [with T = facebook::jsi::Value&]’:
../API/hermes/hermes.cpp:726:31:   required from here
../API/jsi/jsi/jsi.h:963:49: error: no matching function for call to ‘facebook::jsi::Value::kindOf(facebook::jsi::Value&)’
  963 |   /* implicit */ Value(T&& other) : Value(kindOf(other)) {
      |                                           ~~~~~~^~~~~~~
../API/jsi/jsi/jsi.h:1176:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::Symbol&)’
 1176 |   constexpr static ValueKind kindOf(const Symbol&) {
      |                              ^~~~~~
../API/jsi/jsi/jsi.h:1176:37: note:   no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::Symbol&’
 1176 |   constexpr static ValueKind kindOf(const Symbol&) {
      |                                     ^~~~~~~~~~~~~
../API/jsi/jsi/jsi.h:1179:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::String&)’
 1179 |   constexpr static ValueKind kindOf(const String&) {
      |                              ^~~~~~
../API/jsi/jsi/jsi.h:1179:37: note:   no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::String&’
 1179 |   constexpr static ValueKind kindOf(const String&) {
      |                                     ^~~~~~~~~~~~~
../API/jsi/jsi/jsi.h:1182:30: note: candidate: ‘static constexpr facebook::jsi::Value::ValueKind facebook::jsi::Value::kindOf(const facebook::jsi::Object&)’
 1182 |   constexpr static ValueKind kindOf(const Object&) {
      |                              ^~~~~~
../API/jsi/jsi/jsi.h:1182:37: note:   no known conversion for argument 1 from ‘facebook::jsi::Value’ to ‘const facebook::jsi::Object&’
 1182 |   constexpr static ValueKind kindOf(const Object&) {
      |                                     ^~~~~~~~~~~~~
../API/jsi/jsi/jsi.h:966:47: error: static assertion failed: Value cannot be implicitly move-constructed from this type
  965 |         std::is_base_of<Symbol, T>::value ||
      |                                     ~~~~~~~~
  966 |             std::is_base_of<String, T>::value ||
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  967 |             std::is_base_of<Object, T>::value,
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../API/jsi/jsi/jsi.h:966:47: note: ‘((((bool)std::integral_constant<bool, false>::value) || ((bool)std::integral_constant<bool, false>::value)) || ((bool)std::integral_constant<bool, false>::value))’ evaluates to false
../API/jsi/jsi/jsi.h:969:5: error: new cannot be applied to a reference type
  969 |     new (&data_.pointer) T(std::move(other));
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

Among other things, this puts the actual error on the consuming code where the
error is, which results in those nice red squiggles under it while typing to
tell you that you screwed up.

X-link: facebook/hermes#526

Test Plan: This should only convert one type of compiler error to another, so existing tests should be sufficient.

Reviewed By: tmikov

Differential Revision: D46005344

Pulled By: neildhar

fbshipit-source-id: 194e0483177770df578cb864281d66c88d4cdb7e
  • Loading branch information
RedBeard0531 authored and facebook-github-bot committed May 22, 2023
1 parent 11e80b6 commit d470dee
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions packages/react-native/ReactCommon/jsi/jsi/jsi.h
Original file line number Diff line number Diff line change
Expand Up @@ -1098,14 +1098,14 @@ class JSI_EXPORT Value {
}

/// Moves a Symbol, String, or Object rvalue into a new JS value.
template <typename T>
template <
typename T,
typename = std::enable_if_t<
std::is_base_of<Symbol, T>::value ||
std::is_base_of<BigInt, T>::value ||
std::is_base_of<String, T>::value ||
std::is_base_of<Object, T>::value>>
/* implicit */ Value(T&& other) : Value(kindOf(other)) {
static_assert(
std::is_base_of<Symbol, T>::value ||
std::is_base_of<BigInt, T>::value ||
std::is_base_of<String, T>::value ||
std::is_base_of<Object, T>::value,
"Value cannot be implicitly move-constructed from this type");
new (&data_.pointer) T(std::move(other));
}

Expand Down

0 comments on commit d470dee

Please sign in to comment.