general code improvement #60
Conversation
|
ci failing isn't from me |
There was a problem hiding this comment.
Thanks for working on cleanup. Left some minor feedback. After that's addressed we can land this.
Please update your PR body to include a summary of your changes.
Doesn't need to mention everything, just the main ideas.
That summary will then be used for the final rebased commit message.
| @@ -10,6 +10,7 @@ edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| thiserror = "1" | |||
| bytemuck = { version = "1.16.1", features = ["must_cast", "extern_crate_alloc"] } | |||
There was a problem hiding this comment.
No need for the additional dependency.
We should also eventually remove thiserror (doesn't need to happen in this PR)
There was a problem hiding this comment.
wait this should have been a test dep my bad, I was messing with utf-16 being 0 copy on big endian systems but then was like its just a bunch of added complexity I can still do it tho if you're fine with it
| @@ -1,18 +0,0 @@ | |||
| # Drag an Drop PSD Demo | |||
There was a problem hiding this comment.
Why were these files removed from the example?
| impl Default for AppWrapper { | ||
| fn default() -> Self { | ||
| AppWrapper::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
Can remove this if it is unused.
There was a problem hiding this comment.
this is to stop a clippy lint, we can allow that if you so wish, or make AppWrapper not pub
| @@ -147,7 +147,7 @@ impl Psd { | |||
| // Methods for working with layers | |||
| impl Psd { | |||
| /// Get all of the layers in the PSD | |||
| pub fn layers(&self) -> &Vec<PsdLayer> { | |||
| pub fn layers(&self) -> &[PsdLayer] { | |||
There was a problem hiding this comment.
If there's a good reason to make these return &[T] we can consider that.
Otherwise, this is an unnecessary breaking change.
Can change these back to returning &Vec<T>.
| for alpha in rgba.iter_mut().skip(3).step_by(4) { | ||
| *alpha = 255; |
There was a problem hiding this comment.
Less clear and potentially slower in unoptimized builds. So, doesn't seem like an improvement. Can revert.
| @@ -20,7 +20,7 @@ impl SlicesImageResource { | |||
| &self.name | |||
| } | |||
|
|
|||
| pub fn descriptors(&self) -> &Vec<DescriptorStructure> { | |||
| pub fn descriptors(&self) -> &[DescriptorStructure] { | |||
There was a problem hiding this comment.
Can revert. If we want to introduce a breaking change it should be justified.
There was a problem hiding this comment.
I mean, just bump up the semver version?????, and on most cases this works just fine, and this helps inference, as there are basically no methods on &Vec and if anyone is expecting &Vec they should use &[T]
| self.groups.insert(group.id, group); | ||
| } | ||
|
|
||
| /// Get the group ID's in order (from bottom to top in a PSD file). | ||
| pub fn group_ids_in_order(&self) -> &Vec<u32> { | ||
| pub fn group_ids_in_order(&self) -> &[u32] { |
There was a problem hiding this comment.
Can revert. If we want to introduce a breaking change it should be justified.
| // since we are a cursor over a [u8] | ||
| // that means the total amount of bytes is less than isize::MAX | ||
| // therefore the biggest index is isize::MAX | ||
| // also meaning that it can be safely cast to a usize |
There was a problem hiding this comment.
Let's clarify why this is true.
As in, why does a cursor over a [u8] have isize::MAX bytes?
There was a problem hiding this comment.
from the standard library it states:
*The total size len * mem::size_of::<T>() of the slice must be no larger than isize::MAX, and adding that size to data must not “wrap around” the address space. See the safety documentation of pointer::offset.
and this also applies to all types, all allocations/types/abstract machine objects have to not wrap around the address space, so anything in memory must not be larger than isize::MAX bytes
| pub fn seek(&mut self, pos: u64) { | ||
| self.cursor.set_position(pos); | ||
| pub fn seek(&mut self, pos: usize) { | ||
| // see Self::position for more info |
There was a problem hiding this comment.
More info about what? Unless you're on a 128 bit architecture and your PSD has more than u64::MAX bytes this cast is always ok.
There was a problem hiding this comment.
Well yeah I'm explaining why its fine, I'm saying if its in memory that for now (IIRC rust doesnt support 128 bit arch yet) means pos < u64::MAX so the cast is fine
| } | ||
|
|
||
| pixels | ||
| bytemuck::allocation::cast_vec(vec![color; 8 * 8]) |
There was a problem hiding this comment.
Can remove the bytemuck dep. Not worth the extra dep for just this.
With all due respect the code majorly sucks, but this is a little better, I mean... I can't do a full rewrite, just redid the low hanging fruit