[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25368 [Core Tor/Tor]: Update the Tor Rust coding standards for new types
#25368: Update the Tor Rust coding standards for new types
----------------------------------------+----------------------------------
Reporter: teor | Owner: teor
Type: defect | Status: merge_ready
Priority: Medium | Milestone: Tor:
| 0.3.4.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: doc, rust, review-group-34 | Actual Points:
Parent ID: #23061 | Points: 0.5
Reviewer: isis | Sponsor:
----------------------------------------+----------------------------------
Changes (by isis):
* status: needs_review => merge_ready
Comment:
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
[https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-
numbers-2012-edition/ 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.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25368#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs