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

Implement PartialEq for Packed*Array and Eq where appropriate #122

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

ttencate
Copy link
Contributor

See #33.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Feb 12, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 12, 2023

Thanks, looks nice! 👍

The named keys for the macro are mostly for expressivity reasons, or is it needed for parsing disambiguation? Arguably some parts are less readable now, because the symbols are no longer aligned. On the other hand, it's nice ensure it's at the right argument position.

-    i64,
-    i64,
-    packed_int64_array_construct_default,
-    packed_int64_array_construct_copy,
-    packed_int64_array_from_array,
-    packed_int64_array_destroy,
-    packed_int64_array_operator_index,
-    packed_int64_array_operator_index_const,
+    argument_type: i64,
+    return_type: i64,
+    from_array: packed_int64_array_from_array,
+    operator_index: packed_int64_array_operator_index,
+    operator_index_const: packed_int64_array_operator_index_const,
+    trait_impls: {
+        Default => packed_int64_array_construct_default;
+        Clone => packed_int64_array_construct_copy;
+        Drop => packed_int64_array_destroy;
+        Eq => packed_int64_array_operator_equal;
+    },

Anyway, that's bikeshedding, no need to fix 🙂 In general, I wouldn't spend too much time on making this macro look nice, as we're going to rewrite quite a bit once the codegen provides genericity over packed arrays.

@Bromeon
Copy link
Member

Bromeon commented Feb 12, 2023

bors try

bors bot added a commit that referenced this pull request Feb 12, 2023
@bors
Copy link
Contributor

bors bot commented Feb 12, 2023

try

Build succeeded:

@ttencate
Copy link
Contributor Author

The named keys for the macro are mostly for expressivity reasons, or is it needed for parsing disambiguation?

Readability. The argument list got horribly long and it was hard to tell what each argument did. Too bad about the vertical alignment 🤷

@Bromeon
Copy link
Member

Bromeon commented Feb 12, 2023

Fair enough, thanks again! 🙂

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 12, 2023

Build succeeded:

@bors bors bot merged commit da04ee1 into godot-rust:master Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants