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

Aggressively use let-else and combinators to reduce indentation #7104

Closed
wants to merge 3 commits into from
Closed

Aggressively use let-else and combinators to reduce indentation #7104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc39be8
Use let-else on Options
james7132 Jan 6, 2023
edf9b4c
Use let-else for Result
james7132 Jan 6, 2023
66d27f2
Fix CI
james7132 Jan 6, 2023
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: 2 additions & 5 deletions crates/bevy_animation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,8 @@ fn verify_no_ancestor_player(
if maybe_player.is_some() {
return false;
}
if let Some(parent) = parent {
current = parent.get();
} else {
return true;
}
let Some(parent) = parent else { return true };
current = parent.get();
}
}

Expand Down
23 changes: 11 additions & 12 deletions crates/bevy_app/src/plugin_group.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,18 +174,17 @@ impl PluginGroupBuilder {
/// Panics if one of the plugin in the group was already added to the application.
pub fn finish(mut self, app: &mut App) {
for ty in &self.order {
if let Some(entry) = self.plugins.remove(ty) {
if entry.enabled {
debug!("added plugin: {}", entry.plugin.name());
if let Err(AppError::DuplicatePlugin { plugin_name }) =
app.add_boxed_plugin(entry.plugin)
{
panic!(
"Error adding plugin {} in group {}: plugin was already added in application",
plugin_name,
self.group_name
);
}
let Some(entry) = self.plugins.remove(ty) else { continue };
if entry.enabled {
debug!("added plugin: {}", entry.plugin.name());
if let Err(AppError::DuplicatePlugin { plugin_name }) =
app.add_boxed_plugin(entry.plugin)
{
panic!(
"Error adding plugin {} in group {}: plugin was already added in application",
plugin_name,
self.group_name
);
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions crates/bevy_app/src/schedule_runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,8 @@ impl Plugin for ScheduleRunnerPlugin {
#[cfg(not(target_arch = "wasm32"))]
{
while let Ok(delay) = tick(&mut app, wait) {
if let Some(delay) = delay {
std::thread::sleep(delay);
}
let Some(delay) = delay else { continue };
std::thread::sleep(delay);
}
}

Expand Down
25 changes: 11 additions & 14 deletions crates/bevy_asset/src/asset_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,20 +512,17 @@ impl AssetServer {
let asset_sources = self.server.asset_sources.read();
let asset_lifecycles = self.server.asset_lifecycles.read();
for potential_free in potential_frees.drain(..) {
if let Some(&0) = ref_counts.get(&potential_free) {
let type_uuid = match potential_free {
HandleId::Id(type_uuid, _) => Some(type_uuid),
HandleId::AssetPathId(id) => asset_sources
.get(&id.source_path_id())
.and_then(|source_info| source_info.get_asset_type(id.label_id())),
};

if let Some(type_uuid) = type_uuid {
if let Some(asset_lifecycle) = asset_lifecycles.get(&type_uuid) {
asset_lifecycle.free_asset(potential_free);
}
}
}
let Some(&0) = ref_counts.get(&potential_free) else { continue };
let type_uuid = match potential_free {
HandleId::Id(type_uuid, _) => Some(type_uuid),
HandleId::AssetPathId(id) => asset_sources
.get(&id.source_path_id())
.and_then(|source_info| source_info.get_asset_type(id.label_id())),
};

let Some(type_uuid) = type_uuid else { continue } ;
let Some(asset_lifecycle) = asset_lifecycles.get(&type_uuid) else { continue };
asset_lifecycle.free_asset(potential_free);
}
}
}
Expand Down
8 changes: 3 additions & 5 deletions crates/bevy_asset/src/debug_asset_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,9 @@ pub(crate) fn sync_debug_assets<T: Asset + Clone>(
AssetEvent::Created { handle } | AssetEvent::Modified { handle } => handle,
AssetEvent::Removed { .. } => continue,
};
if let Some(handle) = handle_map.handles.get(debug_handle) {
if let Some(debug_asset) = debug_assets.get(debug_handle) {
assets.set_untracked(handle, debug_asset.clone());
}
}
let Some(handle) = handle_map.handles.get(debug_handle) else { continue };
let Some(debug_asset) = debug_assets.get(debug_handle) else { continue };
assets.set_untracked(handle, debug_asset.clone());
}
}

Expand Down
65 changes: 29 additions & 36 deletions crates/bevy_asset/src/io/file_asset_io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,17 @@ impl FileAssetIo {
/// If the `CARGO_MANIFEST_DIR` environment variable is set, then its value will be used
/// instead. It's set by cargo when running with `cargo run`.
pub fn get_base_path() -> PathBuf {
if let Ok(manifest_dir) = env::var("CARGO_MANIFEST_DIR") {
PathBuf::from(manifest_dir)
} else {
env::current_exe()
.map(|path| {
path.parent()
.map(|exe_parent_path| exe_parent_path.to_owned())
.unwrap()
})
.unwrap()
}
env::var("CARGO_MANIFEST_DIR")
.map(|manifest_dir| PathBuf::from(manifest_dir))
.unwrap_or_else(|_| {
env::current_exe()
.map(|path| {
path.parent()
.map(|exe_parent_path| exe_parent_path.to_owned())
.unwrap()
})
.unwrap()
})
Comment on lines +68 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.unwrap_or_else(|_| {
env::current_exe()
.map(|path| {
path.parent()
.map(|exe_parent_path| exe_parent_path.to_owned())
.unwrap()
})
.unwrap()
})
.or_else(|| {
env::current_exe()
.and_then(|path| path.parent())
.and_then(|parent_path| parent_path.to_owned())
}).unwrap()

Copy link
Member Author

Choose a reason for hiding this comment

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

unwrap_or_else does not ever panic, unless you panic in the closure yourself. Neither will or_else into unwrap, but it will create unreachable code for panicking that may not get optimized out under certain configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

or_else into unwrap can panic if the closure returns None

}

/// Returns the root directory where assets are loaded from.
Expand Down Expand Up @@ -166,34 +166,27 @@ impl AssetIo for FileAssetIo {
))]
pub fn filesystem_watcher_system(asset_server: Res<AssetServer>) {
let mut changed = HashSet::default();
let asset_io =
if let Some(asset_io) = asset_server.server.asset_io.downcast_ref::<FileAssetIo>() {
asset_io
} else {
return;
let Some(asset_io) = asset_server.server.asset_io.downcast_ref::<FileAssetIo>() else { return };
let Some(ref watcher) = *asset_io.filesystem_watcher.read() else { return };
loop {
let event = match watcher.receiver.try_recv() {
Ok(result) => result.unwrap(),
Err(TryRecvError::Empty) => break,
Err(TryRecvError::Disconnected) => panic!("FilesystemWatcher disconnected."),
};
let watcher = asset_io.filesystem_watcher.read();
if let Some(ref watcher) = *watcher {
loop {
let event = match watcher.receiver.try_recv() {
Ok(result) => result.unwrap(),
Err(TryRecvError::Empty) => break,
Err(TryRecvError::Disconnected) => panic!("FilesystemWatcher disconnected."),
};
if let notify::event::Event {
kind: notify::event::EventKind::Modify(_),
paths,
..
} = event
{
for path in &paths {
if !changed.contains(path) {
let relative_path = path.strip_prefix(&asset_io.root_path).unwrap();
let _ = asset_server.load_untracked(relative_path.into(), true);
}
if let notify::event::Event {
kind: notify::event::EventKind::Modify(_),
paths,
..
} = event
{
for path in &paths {
if !changed.contains(path) {
let relative_path = path.strip_prefix(&asset_io.root_path).unwrap();
let _ = asset_server.load_untracked(relative_path.into(), true);
}
changed.extend(paths);
}
changed.extend(paths);
}
}
}
21 changes: 10 additions & 11 deletions crates/bevy_asset/src/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,20 +235,19 @@ impl_downcast!(AssetLifecycle);

impl<T: AssetDynamic> AssetLifecycle for AssetLifecycleChannel<T> {
fn create_asset(&self, id: HandleId, asset: Box<dyn AssetDynamic>, version: usize) {
if let Ok(asset) = asset.downcast::<T>() {
self.sender
.send(AssetLifecycleEvent::Create(AssetResult {
asset,
id,
version,
}))
.unwrap();
} else {
let asset = asset.downcast::<T>().unwrap_or_else(|_| {
panic!(
"Failed to downcast asset to {}.",
std::any::type_name::<T>()
);
}
)
});
Comment on lines +238 to +243
Copy link
Contributor

Choose a reason for hiding this comment

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

        let Some(asset) = asset.downcast::<T>() else {
            panic!(
                "Failed to downcast asset to {}.",
                std::any::type_name::<T>()
            )
        };

or even

        let asset = asset.downcast::<T>().expect(
            format!(
                "Failed to downcast asset to {}.",
                std::any::type_name::<T>()
            )
        );

but the latter may have performance impact of formatting even when we're not going to panic

self.sender
.send(AssetLifecycleEvent::Create(AssetResult {
asset,
id,
version,
}))
.unwrap();
}

fn free_asset(&self, id: HandleId) {
Expand Down
5 changes: 2 additions & 3 deletions crates/bevy_audio/src/audio_output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,8 @@ pub fn play_queued_audio_system<Source: Asset + Decodable>(
mut audio: ResMut<Audio<Source>>,
mut sinks: ResMut<Assets<AudioSink>>,
) {
if let Some(audio_sources) = audio_sources {
audio_output.try_play_queued(&*audio_sources, &mut *audio, &mut sinks);
};
let Some(audio_sources) = audio_sources else { return };
audio_output.try_play_queued(&*audio_sources, &mut *audio, &mut sinks);
}

/// Asset controlling the playback of a sound
Expand Down
Loading