#25368 closed defect (fixed)

Update the Tor Rust coding standards for new types

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: doc, rust, review-group-34
Cc: catalyst Actual Points:
Parent ID: #23061 Points: 0.5
Reviewer: isis Sponsor:

Description

I added some new types to the rust coding standards.

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

Child Tickets

Change History (12)

comment:1 Changed 22 months ago by teor

Parent ID: #23061

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

I'll need isis' help with Stringlist.

comment:2 Changed 22 months ago by teor

Status: assignedneeds_review

comment:3 Changed 22 months ago by nickm

Keywords: review-group-34 added

comment:4 Changed 21 months ago by dgoulet

Reviewer: isis

Reviewer for week 03/19

comment:5 Changed 21 months ago by isis

Status: needs_reviewmerge_ready

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.

comment:6 Changed 21 months ago by nickm

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?

comment:7 Changed 21 months ago by nickm

(merged)

comment:8 Changed 21 months ago by catalyst

Cc: catalyst added

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

comment:9 in reply to:  5 ; Changed 21 months ago by teor

Owner: teor deleted
Status: merge_readyassigned

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

comment:10 in reply to:  9 Changed 21 months ago by isis

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.

comment:11 Changed 21 months ago by isis

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

comment:12 Changed 21 months ago by isis

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.