Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion godot-codegen/src/generator/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,12 @@ fn make_enum_as_str(enum_: &Enum) -> TokenStream {

/// Creates implementations for bitwise operators for the given enum.
///
/// Currently, this is just [`BitOr`](std::ops::BitOr) for bitfields but that could be expanded in the future.
/// Currently, for bitfields this is the following:
/// - [`BitOr`][std::ops::BitOr]
/// - [`BitOrAssign`][std::ops::BitOrAssign]
///
/// While it would be possible to expand these with additional bitwise operations such as `BitAnd` and `Not`, the preferred way to manipulate
/// flags is to use the `with` and `without` methods rather than using the bitwise operators themselves.
fn make_enum_bitwise_operators(enum_: &Enum, enum_bitmask: Option<&RustTy>) -> TokenStream {
let name = &enum_.name;

Expand Down
10 changes: 10 additions & 0 deletions godot-core/src/obj/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,16 @@ pub trait EngineBitfield: Copy + 'static {
/// }
/// ```
fn all_constants() -> &'static [EnumConstant<Self>];

/// Returns the flag(s) from `self` combined with the flag(s) from `add_flags` arg.
fn with(self, add_flags: Self) -> Self {
Self::from_ord(self.ord() | add_flags.ord())
}

/// Returns the flag(s) from `self`, except for any that were present in the `remove_flags` arg.
fn without(self, remove_flags: Self) -> Self {
Self::from_ord(self.ord() & !remove_flags.ord())
}
}

/// Trait for enums that can be used as indices in arrays.
Expand Down
73 changes: 73 additions & 0 deletions itest/rust/src/object_tests/bitfield_ops_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright (c) godot-rust; Bromeon and contributors.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use godot::classes::node::DuplicateFlags;
use godot::obj::EngineBitfield;

use crate::framework::itest;

Comment thread
rsethc marked this conversation as resolved.
/// Necessarily since `from_ord` is not `const`.
fn no_flags() -> DuplicateFlags {
// Could use `Default::default()` here. Until it is broken by `!1630`, funnily enough.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Could use `Default::default()` here. Until it is broken by `!1630`, funnily enough.
// Avoid `Default::default()` as its value and presence might change in a future version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But actually I think there's not much point in having this additional indirection, just use from_ord(0) inline -- see below.

DuplicateFlags::from_ord(0)
}

const SIGNALS: DuplicateFlags = DuplicateFlags::SIGNALS;
const GROUPS: DuplicateFlags = DuplicateFlags::GROUPS;
const USE_INSTANTIATION: DuplicateFlags = DuplicateFlags::USE_INSTANTIATION;

#[itest]
fn bitfield_ops_with() {
let no_flags = no_flags();

assert_eq!(no_flags.with(USE_INSTANTIATION), USE_INSTANTIATION);

assert_eq!(
GROUPS.with(SIGNALS),
DuplicateFlags::from_ord(GROUPS.ord() | SIGNALS.ord())
);

assert_eq!(GROUPS.with(GROUPS), GROUPS);

assert_eq!(GROUPS.with(GROUPS.with(SIGNALS)), GROUPS.with(SIGNALS));

let with_then_with = GROUPS.with(SIGNALS).with(USE_INSTANTIATION);
assert!(with_then_with.is_set(GROUPS));
assert!(with_then_with.is_set(SIGNALS));
assert!(with_then_with.is_set(USE_INSTANTIATION));

assert_eq!(GROUPS.with(no_flags), GROUPS);
}
Comment on lines +19 to +44

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test pattern is inconsistent. In some cases you verify based on the numeric values, other cases with is_set (which is strictly weaker because it doesn't account for other bits being set).

Also, you can just use SCRIPTS instead of USE_INSTANTIATION so it's easier to read. And we can even go one step further to check against the numeric values -- they must be stable for compatibility.

Then next thing, you can group all operations yielding the same result, there's no point doing NO_FLAGS.with(x) check at the start and y.with(NO_FLAGS) at the end, and x being different than y 😉

It's a bit nitpicky, but I think it really helps comprehension. Here's a more compact version. I even added two checks for commutativity.

Suggested change
const SIGNALS: DuplicateFlags = DuplicateFlags::SIGNALS;
const GROUPS: DuplicateFlags = DuplicateFlags::GROUPS;
const USE_INSTANTIATION: DuplicateFlags = DuplicateFlags::USE_INSTANTIATION;
#[itest]
fn bitfield_ops_with() {
let no_flags = no_flags();
assert_eq!(no_flags.with(USE_INSTANTIATION), USE_INSTANTIATION);
assert_eq!(
GROUPS.with(SIGNALS),
DuplicateFlags::from_ord(GROUPS.ord() | SIGNALS.ord())
);
assert_eq!(GROUPS.with(GROUPS), GROUPS);
assert_eq!(GROUPS.with(GROUPS.with(SIGNALS)), GROUPS.with(SIGNALS));
let with_then_with = GROUPS.with(SIGNALS).with(USE_INSTANTIATION);
assert!(with_then_with.is_set(GROUPS));
assert!(with_then_with.is_set(SIGNALS));
assert!(with_then_with.is_set(USE_INSTANTIATION));
assert_eq!(GROUPS.with(no_flags), GROUPS);
}
const SIGNALS: DuplicateFlags = DuplicateFlags::SIGNALS; // 1
const GROUPS: DuplicateFlags = DuplicateFlags::GROUPS; // 2
const SCRIPTS: DuplicateFlags = DuplicateFlags::SCRIPTS; // 4
#[itest]
fn bitfield_ops_with() {
let no_flags = DuplicateFlags::from_ord(0);
assert_eq!(no_flags.with(GROUPS).ord(), 2);
assert_eq!(GROUPS.with(no_flags).ord(), 2);
assert_eq!(GROUPS.with(GROUPS).ord(), 2);
assert_eq!(GROUPS.with(SIGNALS).ord(), 1 | 2);
assert_eq!(GROUPS.with(GROUPS.with(SIGNALS)).ord(), 1 | 2);
assert_eq!(GROUPS.with(GROUPS).with(SIGNALS).ord(), 1 | 2);
assert_eq!(GROUPS.with(SIGNALS).with(SCRIPTS).ord(), 1 | 2 | 4);
}


#[itest]
fn bitfield_ops_without() {
let no_flags = no_flags();

assert_eq!(USE_INSTANTIATION.without(USE_INSTANTIATION), no_flags);

assert_eq!(SIGNALS.with(GROUPS).without(SIGNALS), GROUPS);

assert_eq!(GROUPS.without(SIGNALS), GROUPS);

assert_eq!(
SIGNALS
.with(GROUPS)
.without(SIGNALS.with(USE_INSTANTIATION)),
GROUPS
);

let without_then_without = GROUPS
.with(SIGNALS)
.with(USE_INSTANTIATION)
.without(SIGNALS)
.without(USE_INSTANTIATION);
assert!(without_then_without.is_set(GROUPS));
assert!(!without_then_without.is_set(SIGNALS));
assert!(!without_then_without.is_set(USE_INSTANTIATION));

assert_eq!(GROUPS.without(no_flags), GROUPS);
}
Comment on lines +46 to +73

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#[itest]
fn bitfield_ops_without() {
let no_flags = no_flags();
assert_eq!(USE_INSTANTIATION.without(USE_INSTANTIATION), no_flags);
assert_eq!(SIGNALS.with(GROUPS).without(SIGNALS), GROUPS);
assert_eq!(GROUPS.without(SIGNALS), GROUPS);
assert_eq!(
SIGNALS
.with(GROUPS)
.without(SIGNALS.with(USE_INSTANTIATION)),
GROUPS
);
let without_then_without = GROUPS
.with(SIGNALS)
.with(USE_INSTANTIATION)
.without(SIGNALS)
.without(USE_INSTANTIATION);
assert!(without_then_without.is_set(GROUPS));
assert!(!without_then_without.is_set(SIGNALS));
assert!(!without_then_without.is_set(USE_INSTANTIATION));
assert_eq!(GROUPS.without(no_flags), GROUPS);
}
#[itest]
fn bitfield_ops_without() {
let no_flags = DuplicateFlags::from_ord(0);
assert_eq!(no_flags.without(no_flags).ord(), 0);
assert_eq!(GROUPS.without(GROUPS).ord(), 0);
assert_eq!(GROUPS.without(SIGNALS).ord(), 2);
assert_eq!(SIGNALS.with(GROUPS).without(SIGNALS).ord(), 2);
assert_eq!(SIGNALS.with(GROUPS).without(SIGNALS.with(SCRIPTS)).ord(), 2);
let all = GROUPS.with(SIGNALS).with(SCRIPTS);
assert_eq!(all.without(SIGNALS).ord(), 2 | 4);
assert_eq!(all.without(SIGNALS).without(SCRIPTS).ord(), 2);
}

1 change: 1 addition & 0 deletions itest/rust/src/object_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

mod base_test;
mod bitfield_ops_test;
mod call_deferred_test;
mod class_id_test;
mod class_rename_test;
Expand Down
Loading