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.

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

Allowing all binary C++ operators in WebIDL #22770

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

jrouwe
Copy link
Contributor

@jrouwe jrouwe commented Oct 19, 2024

This expands the list of supported operators by the 'Operator' WebIDL attribute to all binary operators that are supported by C++.

Fixes #9415

@jrouwe jrouwe changed the title Allowing all C++ operators in WebIDL Allowing all binary C++ operators in WebIDL Oct 19, 2024
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. @kripken WDYT?

@kripken
Copy link
Member

kripken commented Oct 21, 2024

Hmm, I think the reason we focused on the += form is because the C++ x += y turns into JS x.add(y). Both of these operations are asymmetrical, with x playing a more central role. OTOH, with this change, we'd have C++ x * y turn into JS x.mul(y), so something symmetrical turns into something asymmetrical. So there might be some risk of confusion there.

But maybe that risk is very small? Anyhow, if there is real-world use case I'm not opposed to it. Out of curiosity @jrouwe can you explain a bit about the use case you have in mind for this? I guess the library has only * and not *=?

@jrouwe
Copy link
Contributor Author

jrouwe commented Oct 21, 2024

The use case is for a quaternion or matrix class.

These classes look like:

class Vector { ... };

class Matrix {
public:
  Vector operator *(const Vector &) { ... };
};

In this example the operation Matrix * Vector => Vector cannot be expressed as an operator *= because the return type is not a Matrix but a Vector.

Obviously I could define a function like MultiplyVector in C++ instead of using operator *, but I want my library (JoltPhysics) to have an intuitive interface in C++:

Matrix matrix = ...;
Vector vector = ...;
Vector result = matrix * vector;

B.t.w. the real class lives here:
https://github.com/jrouwe/JoltPhysics/blob/9fb538ef9b945fe4d1ccab18bf979715fd90a312/Jolt/Math/Quat.h#L160

The IDL lives here:
https://github.com/jrouwe/JoltPhysics.js/blob/fa0df81fb3cffb4946a9e5740ecd68ced00f1fb7/JoltJS.idl#L382-L384

@kripken
Copy link
Member

kripken commented Oct 21, 2024

Thanks for the info @jrouwe , yeah, that makes sense. I agree we should support this.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm with an update to the docs:

https://emscripten.org/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.html#operators

File is at site/source/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.rst.

@jrouwe
Copy link
Contributor Author

jrouwe commented Oct 21, 2024

Documentation updated.

call = '((*%s)[%s%s])' % (cast_self, maybe_deref, args[0])
else:
raise Exception('unfamiliar operator ' + operator)
call = '(*%s %s %s%s)' % (cast_self, operator, maybe_deref, args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Can we error somehow if this is not one of the allowed operators? For example if it is a unary operator. Before this PR we'd error on unsupported things, and it would be good to keep doing so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the list explicit (based on https://en.cppreference.com/w/cpp/language/operators). Note that previously operators would be allowed as long as they contained a '=', so 'x=' would have been a valid operator (but doesn't compile obviously). Those will be caught now as well.

@jrouwe jrouwe force-pushed the all_operators branch 2 times, most recently from 864ba41 to 76d4830 Compare October 22, 2024 19:13
Comment on lines 618 to 620
if any(x == operator for x in ["+", "-", "*", "/", "%", "^", "&", "|", "=",
"<", ">", "+=", "-=", "*=", "/=", "%=", "^=", "&=", "|=", "<<", ">>", ">>=",
"<<=", "==", "!=", "<=", ">=", "<=>", "&&", "||"]):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if any(x == operator for x in ["+", "-", "*", "/", "%", "^", "&", "|", "=",
"<", ">", "+=", "-=", "*=", "/=", "%=", "^=", "&=", "|=", "<<", ">>", ">>=",
"<<=", "==", "!=", "<=", ">=", "<=>", "&&", "||"]):
if operator in ["+", "-", "*", "/", "%", "^", "&", "|", "=",
"<", ">", "+=", "-=", "*=", "/=", "%=", "^=", "&=", "|=", "<<", ">>", ">>=",
"<<=", "==", "!=", "<=", ">=", "<=>", "&&", "||"]:

Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't use python very often... I've merged your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, looks good!

Also erroring on previously allowed, but non-existing operators like 'x='.
@kripken kripken merged commit b53978e into emscripten-core:main Oct 22, 2024
28 checks passed
@jrouwe jrouwe deleted the all_operators branch October 23, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebIDL unsupported operators
3 participants