general code improvement by vrtgs · Pull Request #60 · chinedufn/psd

vrtgs · 2024-07-26T19:37:20Z

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

vrtgs · 2024-07-26T20:13:28Z

ci failing isn't from me

chinedufn

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.

chinedufn · 2024-09-04T10:04:10Z

Cargo.toml


 [dependencies]
 thiserror = "1"
+bytemuck = { version = "1.16.1", features = ["must_cast", "extern_crate_alloc"] }


No need for the additional dependency.

We should also eventually remove thiserror (doesn't need to happen in this PR)

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

chinedufn · 2024-09-04T10:04:23Z

examples/drag-drop-browser/README.md

@@ -1,18 +0,0 @@
-# Drag an Drop PSD Demo


Why were these files removed from the example?

chinedufn · 2024-09-04T10:07:27Z

examples/drag-drop-browser/src/lib.rs

+impl Default for AppWrapper {
+    fn default() -> Self {
+        AppWrapper::new()
+    }
+}


Can remove this if it is unused.

this is to stop a clippy lint, we can allow that if you so wish, or make AppWrapper not pub

chinedufn · 2024-09-04T10:11:13Z

src/lib.rs

 impl Psd {
    /// Get all of the layers in the PSD
-    pub fn layers(&self) -> &Vec<PsdLayer> {
+    pub fn layers(&self) -> &[PsdLayer] {


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>.

chinedufn · 2024-09-04T10:12:59Z

src/psd_channel.rs

+            for alpha in rgba.iter_mut().skip(3).step_by(4) {
+                *alpha = 255;


Less clear and potentially slower in unoptimized builds. So, doesn't seem like an improvement. Can revert.

chinedufn · 2024-09-04T10:20:26Z

src/sections/image_resources_section/image_resource.rs

    }

-    pub fn descriptors(&self) -> &Vec<DescriptorStructure> {
+    pub fn descriptors(&self) -> &[DescriptorStructure] {


Can revert. If we want to introduce a breaking change it should be justified.

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]

chinedufn · 2024-09-04T10:20:57Z

src/sections/layer_and_mask_information_section/groups.rs


    /// 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] {


Can revert. If we want to introduce a breaking change it should be justified.

see comment above

chinedufn · 2024-09-04T10:23:36Z

src/sections/mod.rs

+        // 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


Let's clarify why this is true.

As in, why does a cursor over a [u8] have isize::MAX bytes?

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

chinedufn · 2024-09-04T10:25:06Z

src/sections/mod.rs

-    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


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.

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

chinedufn · 2024-09-04T10:27:57Z

tests/compression.rs

-    }
-
-    pixels
+    bytemuck::allocation::cast_vec(vec![color; 8 * 8])


Can remove the bytemuck dep. Not worth the extra dep for just this.

vrtgs added 3 commits July 26, 2024 21:44

general code improvement

b970b56

general code improvement

f0d15e2

general code improvement

af0d7df

chinedufn requested changes Sep 4, 2024

View reviewed changes

		for alpha in rgba.iter_mut().skip(3).step_by(4) {
		*alpha = 255;

Conversation

vrtgs commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrtgs commented Jul 26, 2024

Uh oh!

chinedufn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vrtgs commented Jul 26, 2024 •

edited

Loading