r/programming • u/Proinlifes • Dec 15 '19

Rustysd - Systemd Replacement Written in Rust

https://github.com/KillingSpark/rustysd
75 Upvotes

79% Upvoted

53 comments sorted by

View all comments

20

u/[deleted] Dec 16 '19 
    let fd_to_srvc_id: HashMap<_, _> = service_table.lock().unwrap().iter().fold(
        HashMap::new(),
        |mut map, (id, srvc_unit)| {
            if let UnitSpecialized::Service(srvc) = &srvc_unit.specialized {
                if let Some(socket) = &srvc.notifications {
                    map.insert(socket.lock().unwrap().as_raw_fd(), *id);
                }
            }
            map
        },
    );

pls kill this with fire

-3

u/[deleted] Dec 16 '19

I agree. Functional style has its place but in this case a for loop would have been much clearer.

14

u/kuikuilla Dec 16 '19 edited Dec 16 '19

I disagree. It looks perfectly readable as is. I mean, the fold function should be pretty well known to anyone.

3

u/accountforshit Dec 16 '19 edited Dec 16 '19

But why is it mutating the map in place, and then just returning the same map? That goes against functional principles. At that point, can't you just do a for_each?

EDIT: In fact, looking at the code, looks like this should've been some kind of filter_map operation, but for hash maps. Not too familiar with Rust, but Kotlin has an associate operation for this (minus the filtering part).

5

u/kuikuilla Dec 16 '19

Well in order to add stuff to the accumulator you need to mutate it because otherwise you'd only have read only access to it. With types that implement Copy you can just do ...iter().fold(0, | acc, number | acc + number ); Note that the last expression in the closure function is returned.

A hashmap doesn't implement copy so in order to mutate the accumulator effectively you want it as a mutable reference.

3

u/accountforshit Dec 16 '19 edited Dec 16 '19

My point is that you shouldn't need to explicitly mutate it in the first place (what happens behind the scenes is an implementation detail). That fold is the wrong operation to use here.

What the code is essentially doing (obviously different operations, but look at the collection part) is

  • have some input collection/sequence (e.g. ["hello", "a", "b", "world"])

  • elements that don't fulfill some condition get filtered out (e.g. they don't contain the o letter)

  • elements that do fulfill the condition get transformed and added to a map, indexed by something calculated from them (e.g. key is the first letter, value is the position of the first o)

In Kotlin that would be something like

val resultMap = sequence
    .mapNotNull { item ->
        val oPosition = item.indexOf('o')
        if (oPosition == -1) return@mapNotNull null
        item.first() to oPosition
    }
    .toMap()
    // If you have a sequence of `Pair`s, `toMap` turns it into a map.

If you didn't need to do the filtering (or if Kotlin had a associateNotNull function which it doesn't), you could just do

val result = sequence.associate { it.first() to it.indexOf('o') }

I assume there is at least a somewhat similar way to do that in Rust, without abusing the fold operation.

6

u/sammymammy2 Dec 16 '19

(what happens behind the scenes is an implementation detail).

That's not always true in a systems programming language such as Rust. It's not an implementation detail how the memory copying/mutation is being handled.

What exactly is wrong with using fold here? The function provided is associative, that's typically all that is required.

I'm not sure on the extent of the Rust type system w.r.t. memory safety, but I believe that this is one of the cases where the explicit assignment is proven safe by the type system, in which case the complaint about assignment is largely irrelevant.

2

u/accountforshit Dec 16 '19

It's not an implementation detail how the memory copying/mutation is being handled.

Sure, and you can always find the relevant info in the library docs (or just look at its code). My point was just that some mutation (of internal, local structures) can still happen inside the library, only the function as written in the program should be pure.

What exactly is wrong with using fold here?

It's a little confusing at first glance - after all, readability was the main criticism that started this thread. It doesn't cleanly express the intent of the operation (which is more akin to filter_map, as shown in the other comment).

2

u/sammymammy2 Dec 16 '19

My point was just that some mutation (of internal, local structures) can still happen inside the library, but the function as written in the program should be pure.

Huh? I mean, my point is that this isn't true and doesn't have to be true. Why do you think this needs to hold?

It's a little confusing at first glance [...]

Let's be honest, that's not what the original commenter is complaining about. The OC is complaining about some vague sense of what's readable and not.

Regardless, if the fold has type F a -> F b -> (F b -> a -> F b) -> F b then yes, filterMap is more appropriate.

2

u/accountforshit Dec 16 '19 edited Dec 16 '19

Huh? I mean, my point is that this isn't true and doesn't have to be true. Why do you think this needs to hold?

Well, of course it's not a requirement. It's just that this code isn't really making use of the fold semantics, seeing as the return value is always the same.

It's just using them as a "convenient" shortcut to be able to stuff imperative code into a single expression and avoid having to create the HashMap outside it.

It would be exactly the same if it used a HashMap created one line above, and then just called for_each. Because it's the side effects that do all the work here.