Please criticise my code

[radiant rose]

https://github.com/dev-ardi/treehash.git

This is supposed to be a tool that iterates through directories in parallel to checksum them.
Any kind of criticism is welcome here, even if it's nitpicky or pedantic - I want to become the best I can.

Performance is probably terrible because I haven't profiled it yet

[azure cove]

good code overall. I only have 2 comments:

if t.is_file() {
    ...
}
if t.is_dir() {
    ...
}

compiler might optimize these anyway, but I think it'd be better to use else if (reduces 1 line!)

if t.is_file() {
    let val = IO::File(entry.path());
    children.push(Tree {
        value: val,
        children: Vec::new(),
    });
}

given most items will probably end up being files instead of dirs, I think it might be a good idea to use Option<Vec<Tree>> for children

[supple ruin]

An empty vector doesn't allocate, so Vec::new() is basically free

Though this representation does imply that you could have something that is a file and has children

[azure cove]

I thought about that (although I wasn't sure), but overall I feel like being more explicit would be better in this case

[supple ruin]

I'd probably represent the structure as follows:

pub enum Tree<T> {
  Branch(Vec<Self>),
  Leaf(T),
}

[azure cove]

he also stores the path for dirs/branches, so Branch(T, Vec<Self>)

although at this point you'll end up storing more data than by just using a struct, since an extra bit will be needed to distinguish between enums (I think)

[supple ruin]

Ah yeah, if they're storing paths it makes more sense that eay

[azure cove]

Actually, it came randomly to me while I was refactoring another code that this is not true lol

Since if you use branch/node you'll no longer need the enum for Dir/File, meaning they'll end up the same size

[radiant rose]

That's why I went for it, I prefered removing the option complexity for the tradeoff that you need to know that a file has an empty children vector

[radiant rose]

Ah smart

[radiant rose]

Can you please explain this?

[supple ruin]

Enums are the size of their largest variant, plus a "tag" to know which variant is active

The tag sometimes can be optimized into one the variants' data, but that's not always possible

[radiant rose]

With pointer types it should always be optimizable right?

[supple ruin]

Yesn't. Nonnull pointers (which Vec has one of) have a niche, which is sufficient if there is only one other variant and it carries zero-sized data

[radiant rose]

What if there's the need for 2 or more bits?

[supple ruin]

You'd need to use a type that has two bits to spare, which Vec does not.

Also, some theoretically possible optimizations are not performed because optimizing for size can sometimes slow down matching the enum

[radiant rose]

Surely you're not going to use all 64 bits to address memory

[supple ruin]

Actually, you may very well do that. Isn't virtual memory fun

Also remember Vec may be on a 32-bit system

[radiant rose]

That's something I've never understood: If our virtual address space is a complete fabrication why not use addresses in order?

[supple ruin]

Security, usually

ASLR (Addres Space Layout Randomization) is a security thing

[radiant rose]

How does it secure things?

[supple ruin]

Also, hardening. If you, say, never reuse malloced memory, you can trivially detect use-after free or double-free

[supple ruin]

It makes it hard to guess where the code, or the other "important things" are

[radiant rose]

Isn't that trivial information to get?

[supple ruin]

Which in turn means an attacker that somehow got a way to write to memory (maybe a buffer overflow) can't easily know where to write

[supple ruin]

Yes, which is why we have ASLR to make it hard.

[radiant rose]

Idk all i'm saying is that it wouldn't be that bad to reserve a few bits of the address space for this

[supple ruin]

Currently, no CPUs exist that use more than 57 bits of the pointer. This change was made from the previous limit of 48 because some people actually did hit the 48 bit limit.

In the future, they may well extend to the entire pointer.

And Vec doesn't come with funny stipulations about how it's not allowed to exist at certain addresses because the pointer reserves them as scratch space

In fact, it can't have such a stipulation. It would break other stable promises.

[radiant rose]

So you can have enums up until 32 members for free?

[supple ruin]

No. The extra bits are not scratch space

[radiant rose]

What's scratch space?

[supple ruin]

Space that has no particular use, that you can use for your own purposes

In this case, I assume your idea is that you'd pack the discriminant in the 7 spare bits of a pointer.

But they are not actually spare

You're not supposed to use them.

[radiant rose]

why

[supple ruin]

CPU manufacturers say so

[radiant rose]

a

[supple ruin]

More practically, because future CPUs may claim to be backwards compatible, but also use those bits for the actual pointer

Which would ruin your clever encoding scheme

[radiant rose]

How much scratch space do we have in pointers then? only 1 if it's a non nullable pointer?

[supple ruin]

It's not exactly one bit, mind you. it's one bitpattern. It can't store a boolean

[radiant rose]

huh?

[supple ruin]

*const T and *mut T have zero scratch space. NonNull (and thus all the collection types), and &/&mut have one bitpattern

[supple ruin]

There are no pointer types in Rust that reserve a bit

They reserve specifically the null bitpattern.

[radiant rose]

ah I see

[supple ruin]

This means that Option<NonNull<T>> can be optimized to a nullable pointer

But you can't do that for an Option-like enum with two different Nones

[radiant rose]

How do you know that much about the internals? What have you read?

[supple ruin]

I mostly absorbed knowledge from being in this discord, particularly #rust-discussions-3 and #dark-arts. I think we have some information about this in the reference and in the option docs, though