Skip to content
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.

Sign up for GitHub

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

Jump to bottom

embind: Use optional return type for vector and maps. #21102

Merged
merged 2 commits into from
Feb 2, 2024
Merged

embind: Use optional return type for vector and maps. #21102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9b106a
embind: Use optional return type for vector and maps.
brendandahl Jan 17, 2024
a15f422
Merge branch 'main' into tsgen-vector-map
brendandahl Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 52 additions & 7 deletions system/include/emscripten/bind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1875,6 +1875,21 @@ class class_ {
}
};

#if __cplusplus >= 201703L
template<typename T>
void register_optional() {
// Optional types are automatically registered for some internal types so
// only run the register method once so we don't conflict with a user's
// bindings if they also register the optional type.
thread_local bool hasRun;
if (hasRun) {
return;
}
hasRun = true;
internal::_embind_register_optional(internal::TypeID<std::optional<T>>::get(), internal::TypeID<T>::get());
}
#endif

////////////////////////////////////////////////////////////////////////////////
// VECTORS
////////////////////////////////////////////////////////////////////////////////
Expand All @@ -1883,6 +1898,20 @@ namespace internal {

template<typename VectorType>
struct VectorAccess {
// This nearly duplicated code is used for generating more specific TypeScript
// types when using more modern C++ versions.
#if __cplusplus >= 201703L
static std::optional<typename VectorType::value_type> get(
const VectorType& v,
typename VectorType::size_type index
) {
if (index < v.size()) {
return v[index];
} else {
return {};
}
}
#else
static val get(
const VectorType& v,
typename VectorType::size_type index
Expand All @@ -1893,6 +1922,7 @@ struct VectorAccess {
return val::undefined();
}
}
#endif

static bool set(
VectorType& v,
Expand All @@ -1909,6 +1939,9 @@ struct VectorAccess {
template<typename T>
class_<std::vector<T>> register_vector(const char* name) {
typedef std::vector<T> VecType;
#if __cplusplus >= 201703L
register_optional<T>();
#endif

void (VecType::*push_back)(const T&) = &VecType::push_back;
void (VecType::*resize)(const size_t, const T&) = &VecType::resize;
Expand All @@ -1923,13 +1956,6 @@ class_<std::vector<T>> register_vector(const char* name) {
;
}

#if __cplusplus >= 201703L
template<typename T>
void register_optional() {
internal::_embind_register_optional(internal::TypeID<std::optional<T>>::get(), internal::TypeID<T>::get());
}
#endif

////////////////////////////////////////////////////////////////////////////////
// MAPS
////////////////////////////////////////////////////////////////////////////////
Expand All @@ -1938,6 +1964,21 @@ namespace internal {

template<typename MapType>
struct MapAccess {
// This nearly duplicated code is used for generating more specific TypeScript
// types when using more modern C++ versions.
#if __cplusplus >= 201703L
static std::optional<typename MapType::mapped_type> get(
const MapType& m,
const typename MapType::key_type& k
) {
auto i = m.find(k);
if (i == m.end()) {
return {};
} else {
return i->second;
}
}
#else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be a breaking change? I'm not sure what the policy for those are in Embind, but seems somewhat risky as users who already accessed values via the old signature, will have their code not compile after version upgrade. That switching C++ versions changes the signature seems even more risky.

Maybe safer to introduce the new method as get_optional / maybe_get / something like that, and keep the old one unconditionally there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User's wouldn't be directly calling this code. Only the JS code will be invoking this and the return values will be exactly the same as before in JS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... but if it's only called internally by JS, I'm not sure I understand the value of duplicated implementation code? If it cleaned up val in favour of optional, sure, but if we can't remove it, then keeping the val path seems a bit easier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't use optional here then we don't get improved type of number | undefined for typescript when using c++17. I'm hoping at some point we can drop c++11 support and the val code will go away.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, that makes sense.

Can you add an explanatory comment to those modified places (something along the lines "this branch is used for generating more specific TypeScript types" or something) as I imagine someone looking at the code in the future will be also confused about seemingly duplicate functionality.

I'll approve PR now regardless, I just think a comment will be useful to avoid future confusion.

static val get(
const MapType& m,
const typename MapType::key_type& k
Expand All @@ -1949,6 +1990,7 @@ struct MapAccess {
return val(i->second);
}
}
#endif

static void set(
MapType& m,
Expand All @@ -1975,6 +2017,9 @@ struct MapAccess {
template<typename K, typename V>
class_<std::map<K, V>> register_map(const char* name) {
typedef std::map<K,V> MapType;
#if __cplusplus >= 201703L
register_optional<V>();
#endif

size_t (MapType::*size)() const = &MapType::size;
return class_<MapType>(name)
Expand Down
2 changes: 2 additions & 0 deletions test/other/embind_tsgen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ EMSCRIPTEN_BINDINGS(Test) {

register_vector<int>("IntVec");

register_map<int, int>("MapIntInt");

class_<Foo>("Foo").function("process", &Foo::process);

function("global_fn", &global_fn);
Expand Down
13 changes: 11 additions & 2 deletions test/other/embind_tsgen.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,16 @@ export interface IntVec {
push_back(_0: number): void;
resize(_0: number, _1: number): void;
size(): number;
get(_0: number): number | undefined;
set(_0: number, _1: number): boolean;
get(_0: number): any;
delete(): void;
}

export interface MapIntInt {
keys(): IntVec;
get(_0: number): number | undefined;
set(_0: number, _1: number): void;
size(): number;
delete(): void;
}

Expand Down Expand Up @@ -79,6 +87,7 @@ export interface MainModule {
EmptyEnum: {};
enum_returning_fn(): Bar;
IntVec: {new(): IntVec};
MapIntInt: {new(): MapIntInt};
Foo: {};
ClassWithConstructor: {new(_0: number, _1: ValArr): ClassWithConstructor};
ClassWithTwoConstructors: {new(): ClassWithTwoConstructors; new(_0: number): ClassWithTwoConstructors};
Expand All @@ -87,8 +96,8 @@ export interface MainModule {
DerivedClass: {};
a_bool: boolean;
an_int: number;
global_fn(_0: number, _1: number): number;
optional_test(_0: Foo | undefined): number | undefined;
global_fn(_0: number, _1: number): number;
smart_ptr_function(_0: ClassWithSmartPtrConstructor): number;
smart_ptr_function_with_params(foo: ClassWithSmartPtrConstructor): number;
function_with_callback_param(_0: (message: string) => void): number;
Expand Down
Loading