Skip to content

Commit

Permalink
Enable (selected) new cargo clippy lints (#4404)
Browse files Browse the repository at this point in the history
### What

The big highlight is `needless_pass_by_ref_mut` which helps our
parallization effort, which is why I declare this

* part of #1325

Commit by commit - each commit is one added lint & its fixes (or a
failure to do so!)

Selected from
https://rust-lang.github.io/rust-clippy/master/index.html#?groups=cargo,complexity,correctness,deprecated,nursery,pedantic,perf,restriction,style,suspicious&versions=lte:74,gte:72&levels=allow,none

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [app.rerun.io](https://app.rerun.io/pr/4404) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4404)
- [Docs
preview](https://rerun.io/preview/92edc9f673bca7c2eb77d0688d559f2eb09e4b8f/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/92edc9f673bca7c2eb77d0688d559f2eb09e4b8f/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
Wumpf authored Nov 30, 2023
1 parent 3f11639 commit d0100a0
Show file tree
Hide file tree
Showing 17 changed files with 54 additions and 47 deletions.
8 changes: 8 additions & 0 deletions Cranky.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,25 +84,30 @@ warn = [
"clippy::needless_borrow",
"clippy::needless_continue",
"clippy::needless_for_each",
"clippy::needless_pass_by_ref_mut",
"clippy::needless_pass_by_value",
"clippy::negative_feature_names",
"clippy::nonstandard_macro_braces",
"clippy::option_option",
"clippy::path_buf_push_overwrite",
"clippy::ptr_as_ptr",
"clippy::ptr_cast_constness",
"clippy::pub_without_shorthand",
"clippy::rc_mutex",
"clippy::readonly_write_lock",
"clippy::redundant_type_annotations",
"clippy::ref_option_ref",
"clippy::rest_pat_in_fully_bound_structs",
"clippy::same_functions_in_if_condition",
"clippy::semicolon_if_nothing_returned",
"clippy::should_panic_without_expect",
"clippy::significant_drop_tightening",
"clippy::single_match_else",
"clippy::str_to_string",
"clippy::string_add_assign",
"clippy::string_add",
"clippy::string_lit_as_bytes",
"clippy::string_lit_chars_any",
"clippy::string_to_string",
"clippy::suspicious_command_arg_space",
"clippy::suspicious_xor_used_as_pow",
Expand Down Expand Up @@ -158,4 +163,7 @@ allow = [
# Enabling this lint in 2023-04-27 yielded 556 hits.
# Enabling this lint in 2023-09-23 yielded 352 hits
"clippy::unwrap_used",

# It's nice to avoid ref pattern, but there are some situations that are hard (impossible?) to express without.
"clippy::ref_patterns",
]
20 changes: 8 additions & 12 deletions crates/re_data_ui/src/item_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ fn entity_stats_ui(ui: &mut egui::Ui, timeline: &Timeline, stats: &re_arrow_stor

/// Show a component path and make it selectable.
pub fn component_path_button(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
component_path: &ComponentPath,
) -> egui::Response {
Expand All @@ -182,7 +182,7 @@ pub fn component_path_button(

/// Show a component path and make it selectable.
pub fn component_path_button_to(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
text: impl Into<egui::WidgetText>,
component_path: &ComponentPath,
Expand All @@ -193,7 +193,7 @@ pub fn component_path_button_to(
}

pub fn data_blueprint_group_button_to(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
text: impl Into<egui::WidgetText>,
space_view_id: SpaceViewId,
Expand Down Expand Up @@ -233,7 +233,7 @@ pub fn data_blueprint_button_to(
}

pub fn time_button(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
timeline: &Timeline,
value: TimeInt,
Expand Down Expand Up @@ -261,15 +261,15 @@ pub fn time_button(
}

pub fn timeline_button(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
timeline: &Timeline,
) -> egui::Response {
timeline_button_to(ctx, ui, timeline.name().to_string(), timeline)
}

pub fn timeline_button_to(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
text: impl Into<egui::WidgetText>,
timeline: &Timeline,
Expand All @@ -289,7 +289,7 @@ pub fn timeline_button_to(

// TODO(andreas): Move elsewhere, this is not directly part of the item_ui.
pub fn cursor_interact_with_selectable(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
response: egui::Response,
item: Item,
) -> egui::Response {
Expand All @@ -307,11 +307,7 @@ pub fn cursor_interact_with_selectable(
}

// TODO(andreas): Move elsewhere, this is not directly part of the item_ui.
pub fn select_hovered_on_click(
ctx: &mut ViewerContext<'_>,
response: &egui::Response,
items: &[Item],
) {
pub fn select_hovered_on_click(ctx: &ViewerContext<'_>, response: &egui::Response, items: &[Item]) {
re_tracing::profile_function!();

if response.hovered() {
Expand Down
20 changes: 10 additions & 10 deletions crates/re_query/benches/query_benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ fn mono_points(c: &mut Criterion) {
{
let mut group = c.benchmark_group("arrow_mono_points2");
group.throughput(criterion::Throughput::Elements(NUM_POINTS as _));
let mut store = insert_rows(msgs.iter());
let store = insert_rows(msgs.iter());
group.bench_function("query", |b| {
b.iter(|| query_and_visit_points(&mut store, &paths));
b.iter(|| query_and_visit_points(&store, &paths));
});
}
}
Expand All @@ -131,9 +131,9 @@ fn mono_strings(c: &mut Criterion) {
{
let mut group = c.benchmark_group("arrow_mono_strings2");
group.throughput(criterion::Throughput::Elements(NUM_POINTS as _));
let mut store = insert_rows(msgs.iter());
let store = insert_rows(msgs.iter());
group.bench_function("query", |b| {
b.iter(|| query_and_visit_strings(&mut store, &paths));
b.iter(|| query_and_visit_strings(&store, &paths));
});
}
}
Expand All @@ -156,9 +156,9 @@ fn batch_points(c: &mut Criterion) {
{
let mut group = c.benchmark_group("arrow_batch_points2");
group.throughput(criterion::Throughput::Elements(NUM_POINTS as _));
let mut store = insert_rows(msgs.iter());
let store = insert_rows(msgs.iter());
group.bench_function("query", |b| {
b.iter(|| query_and_visit_points(&mut store, &paths));
b.iter(|| query_and_visit_points(&store, &paths));
});
}
}
Expand All @@ -181,9 +181,9 @@ fn batch_strings(c: &mut Criterion) {
{
let mut group = c.benchmark_group("arrow_batch_strings2");
group.throughput(criterion::Throughput::Elements(NUM_POINTS as _));
let mut store = insert_rows(msgs.iter());
let store = insert_rows(msgs.iter());
group.bench_function("query", |b| {
b.iter(|| query_and_visit_strings(&mut store, &paths));
b.iter(|| query_and_visit_strings(&store, &paths));
});
}
}
Expand Down Expand Up @@ -264,7 +264,7 @@ struct SavePoint {
_color: Option<Color>,
}

fn query_and_visit_points(store: &mut DataStore, paths: &[EntityPath]) -> Vec<SavePoint> {
fn query_and_visit_points(store: &DataStore, paths: &[EntityPath]) -> Vec<SavePoint> {
let timeline_frame_nr = Timeline::new("frame_nr", TimeType::Sequence);
let query = LatestAtQuery::new(timeline_frame_nr, (NUM_FRAMES_POINTS as i64 / 2).into());

Expand Down Expand Up @@ -292,7 +292,7 @@ struct SaveString {
_label: Option<Text>,
}

fn query_and_visit_strings(store: &mut DataStore, paths: &[EntityPath]) -> Vec<SaveString> {
fn query_and_visit_strings(store: &DataStore, paths: &[EntityPath]) -> Vec<SaveString> {
let timeline_frame_nr = Timeline::new("frame_nr", TimeType::Sequence);
let query = LatestAtQuery::new(timeline_frame_nr, (NUM_FRAMES_STRINGS as i64 / 2).into());

Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/depth_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ struct DepthTexture {
}

impl DepthTexture {
pub fn spiral(re_ctx: &mut re_renderer::RenderContext, dimensions: glam::UVec2) -> Self {
pub fn spiral(re_ctx: &re_renderer::RenderContext, dimensions: glam::UVec2) -> Self {
let size = (dimensions.x * dimensions.y) as usize;
let mut data = std::iter::repeat(0f32).take(size).collect_vec();
spiral(dimensions).for_each(|(texcoords, d)| {
Expand Down Expand Up @@ -442,7 +442,7 @@ struct AlbedoTexture {
}

impl AlbedoTexture {
pub fn spiral(re_ctx: &mut re_renderer::RenderContext, dimensions: glam::UVec2) -> Self {
pub fn spiral(re_ctx: &re_renderer::RenderContext, dimensions: glam::UVec2) -> Self {
let size = (dimensions.x * dimensions.y) as usize;
let mut rgba8 = std::iter::repeat(0).take(size * 4).collect_vec();
spiral(dimensions).for_each(|(texcoords, d)| {
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/examples/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ impl<E: Example + 'static> Application<E> {
}

#[allow(dead_code)]
pub fn load_rerun_mesh(re_ctx: &mut RenderContext) -> Vec<re_renderer::renderer::MeshInstance> {
pub fn load_rerun_mesh(re_ctx: &RenderContext) -> Vec<re_renderer::renderer::MeshInstance> {
let reader = std::io::Cursor::new(include_bytes!("rerun.obj.zip"));
let mut zip = zip::ZipArchive::new(reader).unwrap();
let mut zipped_obj = zip.by_name("rerun.obj").unwrap();
Expand Down
6 changes: 4 additions & 2 deletions crates/re_renderer/src/file_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,8 @@ mod tests_file_resolver {
}

#[test]
#[should_panic] // TODO(cmc): check error contents
#[allow(clippy::should_panic_without_expect)] // TODO(cmc): check error contents
#[should_panic]
fn cyclic_direct() {
let fs = MemFileSystem::get();
{
Expand Down Expand Up @@ -819,7 +820,8 @@ mod tests_file_resolver {
}

#[test]
#[should_panic] // TODO(cmc): check error contents
#[allow(clippy::should_panic_without_expect)] // TODO(cmc): check error contents
#[should_panic]
fn cyclic_indirect() {
let fs = MemFileSystem::get();
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ mod tests {
let initial_resource_descs = [0, 0, 1, 2, 2, 3];

// Alloc on a new pool always returns a new resource.
allocate_resources(&initial_resource_descs, &mut pool, true);
allocate_resources(&initial_resource_descs, &pool, true);

// After frame maintenance we get used resources.
// Still, no resources were dropped.
Expand All @@ -295,9 +295,9 @@ mod tests {
}

// Allocate the same resources again, this should *not* create any new resources.
allocate_resources(&initial_resource_descs, &mut pool, false);
allocate_resources(&initial_resource_descs, &pool, false);
// Doing it again, it will again create resources.
allocate_resources(&initial_resource_descs, &mut pool, true);
allocate_resources(&initial_resource_descs, &pool, true);

// Doing frame maintenance twice will drop all resources
{
Expand Down Expand Up @@ -360,7 +360,7 @@ mod tests {

fn allocate_resources(
descs: &[u32],
pool: &mut DynamicResourcePool<ConcreteHandle, ConcreteResourceDesc, ConcreteResource>,
pool: &DynamicResourcePool<ConcreteHandle, ConcreteResourceDesc, ConcreteResource>,
expect_allocation: bool,
) {
let drop_counter_before = DROP_COUNTER.with(|c| c.get());
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl SpatialSpaceViewState {

pub fn selection_ui(
&mut self,
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
space_origin: &EntityPath,
spatial_kind: SpatialSpaceViewKind,
Expand Down
2 changes: 1 addition & 1 deletion crates/re_time_panel/src/data_density_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ fn make_brighter(color: Color32) -> Color32 {

fn show_row_ids_tooltip(
ctx: &mut ViewerContext<'_>,
time_ctrl: &mut TimeControl,
time_ctrl: &TimeControl,
egui_ctx: &egui::Context,
item: &Item,
time_range: TimeRange,
Expand Down
6 changes: 3 additions & 3 deletions crates/re_time_panel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl TimePanel {
#[allow(clippy::unused_self)]
fn collapsed_ui(
&mut self,
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
time_ctrl: &mut TimeControl,
) {
Expand Down Expand Up @@ -691,7 +691,7 @@ impl TimePanel {

fn top_row_ui(
&mut self,
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
time_ctrl: &mut TimeControl,
) {
Expand Down Expand Up @@ -773,7 +773,7 @@ fn highlight_timeline_row(

fn collapsed_time_marker_and_time(
ui: &mut egui::Ui,
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
time_ctrl: &mut TimeControl,
) {
let space_needed_for_current_time = match time_ctrl.timeline().typ() {
Expand Down
2 changes: 1 addition & 1 deletion crates/re_time_panel/src/time_ranges_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ impl TimeRangesUi {
}

// Make sure playback time doesn't get stuck between non-continuous regions:
pub fn snap_time_control(&self, ctx: &mut ViewerContext<'_>) {
pub fn snap_time_control(&self, ctx: &ViewerContext<'_>) {
let mut time_ctrl = ctx.rec_cfg.time_ctrl.write();

if time_ctrl.play_state() != PlayState::Playing {
Expand Down
2 changes: 1 addition & 1 deletion crates/re_ui/src/list_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ impl<'a> ListItem<'a> {
// Draw text next to the icon.
let mut text_rect = rect;
text_rect.min.x += collapse_extra + icon_extra;
if let Some(ref button_response) = button_response {
if let Some(button_response) = &button_response {
text_rect.max.x -= button_response.rect.width() + ReUi::text_to_icon_padding();
}

Expand Down
3 changes: 2 additions & 1 deletion crates/re_ui/src/toggle_switch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,14 @@ fn toggle_switch_ui(ui: &mut egui::Ui, on: &mut bool) -> egui::Response {
response
}

// A wrapper that allows the more idiomatic usage pattern: `ui.add(toggle_switch(&mut my_bool))`
/// A wrapper that allows the more idiomatic usage pattern: `ui.add(toggle_switch(&mut my_bool))`
/// iOS-style toggle switch.
///
/// ## Example:
/// ``` ignore
/// ui.add(toggle_switch(&mut my_bool));
/// ```
#[allow(clippy::needless_pass_by_ref_mut)] // False positive, toggle_switch_ui needs &mut
pub fn toggle_switch(on: &mut bool) -> impl egui::Widget + '_ {
move |ui: &mut egui::Ui| toggle_switch_ui(ui, on)
}
4 changes: 2 additions & 2 deletions crates/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ impl AppState {
}

// This must run after any ui code, or other code that tells egui to open an url:
check_for_clicked_hyperlinks(&re_ui.egui_ctx, &mut rec_cfg.selection_state);
check_for_clicked_hyperlinks(&re_ui.egui_ctx, &rec_cfg.selection_state);
}

pub fn recording_config_mut(&mut self, rec_id: &StoreId) -> Option<&mut RecordingConfig> {
Expand Down Expand Up @@ -350,7 +350,7 @@ fn recording_config_entry<'cfgs>(
/// Detect and handle that here.
///
/// Must run after any ui code, or other code that tells egui to open an url.
fn check_for_clicked_hyperlinks(egui_ctx: &egui::Context, selection_state: &mut SelectionState) {
fn check_for_clicked_hyperlinks(egui_ctx: &egui::Context, selection_state: &SelectionState) {
let recording_scheme = "recording://";

let mut path = None;
Expand Down
2 changes: 1 addition & 1 deletion crates/re_viewer/src/ui/selection_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ fn has_data_section(item: &Item) -> bool {
}

fn space_view_button(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
space_view: &re_viewport::SpaceViewBlueprint,
) -> egui::Response {
Expand Down
8 changes: 4 additions & 4 deletions crates/re_viewer/src/ui/visible_history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ fn has_visible_history(
}

pub fn visible_history_ui(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
space_view_class: &SpaceViewClassName,
is_space_view: bool,
Expand Down Expand Up @@ -238,7 +238,7 @@ pub fn visible_history_ui(
}

fn current_range_ui(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut Ui,
current_time: i64,
is_sequence_timeline: bool,
Expand Down Expand Up @@ -278,7 +278,7 @@ fn current_range_ui(

#[allow(clippy::too_many_arguments)]
fn resolved_visible_history_boundary_ui(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
visible_history_boundary: &VisibleHistoryBoundary,
is_sequence_timeline: bool,
Expand Down Expand Up @@ -388,7 +388,7 @@ fn visible_history_boundary_combo_label(

#[allow(clippy::too_many_arguments)]
fn visible_history_boundary_ui(
ctx: &mut ViewerContext<'_>,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
visible_history_boundary: &mut VisibleHistoryBoundary,
is_sequence_timeline: bool,
Expand Down
Loading

0 comments on commit d0100a0

Please sign in to comment.