Update the Tor Rust coding standards for new types

I added some new types to the rust coding standards.

But I think I will need isis' help to explain Stringlist.

changed milestone to %Tor: 0.3.4.x-final

added component::core tor/tor doc milestone::Tor: 0.3.4.x-final parent::23061 points::0.5 priority::medium resolution::fixed review-group-34 reviewer::isis rust severity::normal status::closed type::defect labels

See my branch rust-std on https://github.com/teor2345/tor.git

I'll need isis' help with Stringlist.

Trac:
Parent: N/A to #23061 (moved)

Trac:
Status: assigned to needs_review

Trac:
Keywords: doc rust deleted, rust, doc, review-group-34 added

Reviewer for week 03/19

Trac:
Reviewer: N/A to isis

This looks great! A couple thoughts:

1. I only know enough about floating point arithmetic to stay away from it. That said, I was under the impression that different compilers (and/or different compiler flags) can treat bitwise-identical floating points differently, and I worry in general that—if we allow f{16,32,64}s to cross the FFI boundary, that the C compiler on one side will treat them differently to how the Rust compiler does. I'm not sure how much this matters for the applications where we're using floating points, which seem mostly localised to the system using them (i.e. we're not sending floats over the network and expecting others to interpret them exactly as we do, right?).

2. I can't explain Stringlist; it's always confused me. The way I'd do it is to have direct conversion between a smartlist_t and a Vec<T>, where T is probably an opaque pointer to whatever type in C, or T is only allowed to be a String which we've copied from a non-NULL char* (e.g. impl From<Stringlist> for Vec<String>, or something, and then keep Stringlist private since internally it's a bunch of C types that we don't want propagating into our more Rusty code)… but also I have not thought this through as much as I should yet.

I'm okay with merging this, but also I think we should probably be figuring out a better way to deal with smartlist_t←→Vec<T> conversions.

Trac:
Status: needs_review to merge_ready

Okay. Based on that, I'll merge this. Please close this ticket once either the above issues are addressed, or we have new tickets for them?

(merged)

I talked briefly with komlo about the Stringlist thing and it seems like she wanted to do the simpler thing of a smartlist of strings because doing the generic thing correctly might take significantly more work and not necessarily be right on the first try.

I would be in favor of a ticket for a longer term plan to try to get the generic Rust representation of a smartlist right. I don't have enough background to open a ticket myself on the issue. (Also sorry if I misremember the details of what komlo told me.)

Trac:
Cc: N/A to catalyst

Replying to isis:

This looks great! A couple thoughts:

1. I only know enough about floating point arithmetic to stay away from it. That said, I was under the impression that different compilers (and/or different compiler flags) can treat bitwise-identical floating points differently,

We can write unit tests for issues like this, but I don't think they will affect us.

and I worry in general that—if we allow f{16,32,64}s to cross the FFI boundary, that the C compiler on one side will treat them differently to how the Rust compiler does. I'm not sure how much this matters for the applications where we're using floating points, which seem mostly localised to the system using them

"Raw transmutation from u64.

This is currently identical to transmute::<u64, f64>(v) on all platforms. It turns out this is incredibly portable, for two reasons:

Floats and Ints have the same endianess on all supported platforms. IEEE-754 very precisely specifies the bit layout of floats.

However there is one caveat…"

https://doc.rust-lang.org/std/primitive.f64.html#method.from_bits

The caveat doesn't apply, because we aren't transmitting binary floats over the network. We don't even want to transmit decimal floats over the network. Nor would we ever transmit NaNs over the network.

(i.e. we're not sending floats over the network and expecting others to interpret them exactly as we do, right?).

Trac:
Owner: teor to N/A
Status: merge_ready to assigned

Replying to teor:

Replying to isis: The caveat doesn't apply, because we aren't transmitting binary floats over the network. We don't even want to transmit decimal floats over the network. Nor would we ever transmit NaNs over the network.

(i.e. we're not sending floats over the network and expecting others to interpret them exactly as we do, right?).

Ah okay, good. Then everything sounds fine to me.

I made #25504 (moved) for genericising the smartlist_t/Vec<T> stuff.

Trac:
Resolution: N/A to fixed
Status: assigned to closed

closed

changed time estimate to 4h

mentioned in issue #25504 (moved)

mentioned in issue #25727 (moved)

moved to tpo/core/tor#25368 (closed)

mentioned in issue tpo/core/tor#25504 (closed)

mentioned in issue tpo/core/tor#25727 (closed)