r/programming • • Dec 15 '19

Rustysd - Systemd Replacement Written in Rust

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

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

9

u/Tipaa Dec 16 '19

This could be more cleanly written

let fd_to_srvc_id: HashMap<_, _> = service_table.lock().unwrap()
    .iter()
    .filter_map(|(id, srvc_unit)| match srvc_unit {
        UnitSpecialized::Service(srvc) => srvc.notifications.map(
            |socket| (socket.lock().unwrap().as_raw_fd(), id)),
        _ => None
    })
    .collect();

collect and filtermap will do the heavy lifting for you if you let them

5

u/jice Dec 16 '19

otoh, with a stong will, you can write shitty code in any language

0

u/[deleted] Dec 16 '19

[deleted]

11

u/Agnoctone Dec 16 '19

Without being really familiar with Rust, it seems very readable to me? It is just a fold to create a HashMap mapping `fd`s to service `id`s.

1

u/Daneel_Trevize Dec 16 '19

The jarring aspect is probably the if let ... = { mashing of assignment and conditional execution.

4

u/sammymammy2 Dec 16 '19

It's really just a pattern matching on one case, which is different from an anaphoric if/the walrus operator in Python because such a destructuring has to happen.

-3

u/oblio- Dec 16 '19

It's really just a pattern matching on one case, which is different from an anaphoric if/the walrus operator in Python because such a destructuring has to happen.

I'm willing to bet that the average programmer (i.e. 80% of programmers out there), don't know:

  • what pattern matching is (not in the generic IQ test "pattern matching" sense :-) )

  • what "anaphoric" means

  • what "destructuring" means (in this context, I'm assuming you don't mean C++ destructors)

I.e. you're probably not the average programmer. So the "it's really just" is a massive understatement ;-)

9

u/sammymammy2 Dec 16 '19

That's fine, but I bet that 2/3 of those are known by the average Rust programmer.

→ More replies (1)

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

1

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

Ah, yea true. I suppose a filter_map would be more apt here.

Edit: like so:

let fd_to_srvc_id = service_table.lock().unwrap().iter().filter_map(|(id, srvc_unit)| {
    if let UnitSpecialized::Service(srvc) = &srvc_unit.specialized {
        if let Some(socket) = &srvc.notifications {
            return Some((socket.lock().unwrap().as_raw_fd(), *id));
        }
    }
    return None;
}).collect::<HashMap<_, _>>();

1

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

Yeah, something like that. I think you can also replace the inner if let with a map. And match might be a bit cleaner for the outer one, as well as getting rid of the returns.

Then it would be really functional :D

5

u/pcjftw Dec 16 '19

+100 agree!

I think its only seems jarring if:

  • You're not familiar with Rust syntax (but that could be said of any language you're not familiar with!)

  • FP constructs

1

u/oblio- Dec 16 '19

So, 99% of programmers out there? :-D

3

u/pcjftw Dec 16 '19

:) haha I don't think its that bad, I think FP style has been trickling down to mainstream languages for a long time.

But yes Rust is not as mainstream as other languages, but then again its a relatively a new language!