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

fix some clippy errors #11

Merged
merged 5 commits into from
Mar 6, 2022
Merged

fix some clippy errors #11

merged 5 commits into from
Mar 6, 2022

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Mar 5, 2022

Running cargo clippy currently comes up with a few errors. I fixed some of them in this commit.

A few others come up that I wanted to mention:

  • Errors around borrowed boxes. I think we can replace some of the &Box fields with just & where clippy tells us to.
  • Errors around tabs in doc comments. This can cause some issues for generated doc pages.

I also don't know how strongly you are attached to your current rustfmt.toml settings, but I would prefer to follow rust community style-guides where possible.

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 5, 2022

This is great! Thanks for this.

A few others come up that I wanted to mention:

  • Errors around borrowed boxes. I think we can replace some of the &Box fields with just & where clippy tells us to.
  • Errors around tabs in doc comments. This can cause some issues for generated doc pages.

Yeah I'm fine with fixing these. I meant to get around to the tabs issue for a while but forgot 😅

I also don't know how strongly you are attached to your current rustfmt.toml settings, but I would prefer to follow rust community style-guides where possible.

No attachment. Let's switch to the community style-guidelines 🙂

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 5, 2022

No attachment. Let's switch to the community style-guidelines 🙂

Cool, we can do this in a separate PR since that's a lot of lines to touch.

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 5, 2022

This type is giving a clippy error about type complexity:

/// A resource containing data for all prototypes that need data stored
pub struct ProtoData {
	/// Maps Prototype Name -> Component Type -> HandlePath -> Asset Type -> HandleUntyped
	handles: HashMap<
		String, // Prototype Name
		HashMap<
			TypeId, // Component Type
			HashMap<
				String, // Handle Path
				HashMap<
					Uuid,          // Asset UUID
					HandleUntyped, // Handle
				>,
			>,
		>,
	>,
	prototypes: HashMap<String, Box<dyn Prototypical>>,
}

Do we need to access each layer of this mapping individually? If not we could save some allocations by consolidating the keys into one struct that we could hash on its own. I was thinking something like:

#[derive(Hash)]
pub struct ProtoHandleKey {
  name: String,
  typeId: TypeId,
  path: String,
  uuid: Uuid
}

pub struct ProtoData {
  handles: HashMap<ProtoHandleKey, HandleUntyped>,
  // ...
}

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 5, 2022

Do we need to access each layer of this mapping individually? If not we could save some allocations by consolidating the keys into one struct that we could hash on its own.

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

(As you can see, this was one of my first Rust projects so it has a lot of little improvements that could be made like this haha)

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 6, 2022

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

On second thought, the nested maps actually make more sense because we can key into them with just references. Making a new key type would mean calling .to_string() just to construct keys for lookups. To shut clippy up I just made a type alias for the innermost map.

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 6, 2022

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

On second thought, the nested maps actually make more sense because we can key into them with just references. Making a new key type would mean calling .to_string() just to construct keys for lookups. To shut clippy up I just made a type alias for the innermost map.

That's true. I'd still like to find a way to clean this up in the future. Maybe once I get the reflection rework done, we can see about tidying it up further.

What if we actually create additional types for the others (i.e., type PathUuidMap = HashMap<String, UuidHandleMap>)?

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 6, 2022

It's just aliases, I don't think it really makes it much clearer to do that. If you think it helps readability then we can.

For now all of the clippy errors are cleaned up 👍

@MrGVSV MrGVSV merged commit 64349cc into MrGVSV:main Mar 6, 2022
@B-Reif B-Reif deleted the clippy-fixes branch March 6, 2022 19:57
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.

2 participants