Opened 2 months ago

Closed 3 weeks ago

#27253 closed enhancement (user disappeared)

Decide: rename Size subclasses to indicate unsigned and bit counts

Reported by: dmr Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords: client dev code-improvement
Cc: teor Actual Points:
Parent ID: Points:
Reviewer: atagar Sponsor:

Description

I wanted to make sure this got specifically addressed before a stem.client release, since after that point it might be too late (due to backwards-compatibility concerns).

From 26227#comment:4:

Naming: stem.client.datatype.Size subclasses/attributes

Suggestion:
It might be good to switch CHAR/SHORT/etc. to UCHAR/USHORT/etc.
I don't know what the convention is here, but it may help for readability.

I'm personally used to U<size> to signify unsigned and <size> to signify signed. I think switching to U<size> would make the code potentially easier to read for newcomers from various backgrounds.

Suggestion:
Similarly, it may help to put the bits length in it, too - for the most immediate readability.
So, e.g.:

  • UCHAR8
  • USHORT16
  • etc.

For reference, on a quick glance...

atagar: thoughts?

Child Tickets

Change History (4)

comment:1 Changed 2 months ago by dmr

Reviewer: atagar
Status: newneeds_review

Bumping to needs_review, since that seems most logical to get a decision for this.

comment:2 in reply to:  description Changed 2 months ago by teor

Replying to dmr:

From 26227#comment:4:

Naming: stem.client.datatype.Size subclasses/attributes

Suggestion:
It might be good to switch CHAR/SHORT/etc. to UCHAR/USHORT/etc.
I don't know what the convention is here, but it may help for readability.

I'm personally used to U<size> to signify unsigned and <size> to signify signed. I think switching to U<size> would make the code potentially easier to read for newcomers from various backgrounds.

C uses unsigned types because certain operations (overflow, bit shifting) are defined on unsigned types, but undefined or produce unexpected results on signed types.

Suggestion:
Similarly, it may help to put the bits length in it, too - for the most immediate readability.
So, e.g.:

  • UCHAR8
  • USHORT16
  • etc.

For reference, on a quick glance...

Trunnel is a custom data declaration language, its conventions are likely an abbreviation of the C standard.

Trunnel integer types also happen to have the same names as Rust integer types:
https://doc.rust-lang.org/book/2018-edition/ch03-02-data-types.html#integer-types

These types are defined in the C standard:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdint.h.html

I encourage you to follow python conventions, whatever they are.

atagar: thoughts?

comment:3 Changed 2 months ago by atagar

Hi Dave, sorry about the delay in getting to your pull request! It's review season at work right now so bit stressed, and focusing on that right now.

When it comes to the unsigned flag that is the *one* thing I already gave a little thought to... and honestly reverted since I didn't understand a point in it.

https://gitweb.torproject.org/user/atagar/stem.git/commit/?h=dmr_review

Quite possible thought this is actually for something upcoming?

I wanted to make sure this got specifically addressed before a stem.client release, since after that point it might be too late (due to backwards-compatibility concerns).

Don't fret about stem.client when it comes to the release. We're not vending this module at all in 1.7. We can continue to change it at will.

We *are* vending stem.descriptor.remote's new ORPort capabilities which uses stem.client, but as long as that keeps working we're good.

comment:4 Changed 3 weeks ago by atagar

Resolution: user disappeared
Status: needs_reviewclosed

Resolving our SoP tickets. If you return and would care to push this forward feel free to reopen.

Note: See TracTickets for help on using tickets.