openssl / technical-policies

Mirror of the repository for technical policies, governed by the OTC (OpenSSL Technical Committee)
20 stars 32 forks source link

Deprecate long and add notes on integer types #51

Closed hlandau closed 1 year ago

hlandau commented 2 years ago

Fixes https://github.com/openssl/openssl/issues/18338

hlandau commented 2 years ago

Updated.

hlandau commented 2 years ago

Updated.

hlandau commented 2 years ago

Updated.

hlandau commented 2 years ago

Updated.

hlandau commented 2 years ago

Updated.

hlandau commented 2 years ago

Updated.

mattcaswell commented 2 years ago

There are conflicts in this PR. Please can we get them resolved and then we can start a vote on this.

hlandau commented 2 years ago

Rebased.

mattcaswell commented 2 years ago

@hlandau - there seems to be some unresolved comments from @kroeckx. Can you respond before we take this to a vote?

hlandau commented 2 years ago

@mattcaswell I've responded, I don't think changes are needed.

hlandau commented 2 years ago

Still current, possible candidate for OTC discussion tomorrow.

mattcaswell commented 2 years ago

I have an open action to start a vote on this. I can start that shortly if you wish?

t8m commented 2 years ago

I do not see a need to discuss this again at OTC meeting. It was already discussed and there were no substantial changes since. So IMO the vote can be started.

hlandau commented 2 years ago

Sounds good.

mattcaswell commented 2 years ago

Starting the vote for this: deprecate long and add notes on integer types

mattcaswell commented 2 years ago

Vote: [+1]

t-j-h commented 2 years ago

Vote: [-1]

The recommendation on using int8_t is enough for me to vote against this suggestion. I also see no reason to avoid using long and using size_t for array indexes we know are small makes no sense.

t8m commented 2 years ago

Vote: [+1]

paulidale commented 2 years ago

Vote: [-1]

levitte commented 2 years ago

From the responses, it looks like this isn't really ready, and needs another round of fiddling.

Vote: [0]

levitte commented 2 years ago

Re size_t for all array indexes, I would say that consistency is a factor. It makes no sense to have different index types depending on the size of the array (what would be the pivot size?)

kroeckx commented 2 years ago

I'm voting +1

mattcaswell commented 2 years ago

Current voting position:

  Dmitry     [  ]
  Matt       [+1]
  Pauli      [-1]
  Tim        [-1]
  Richard    [ 0]
  Shane      [  ]
  Tomas      [+1]
  Kurt       [+1]
  Matthias   [  ]
  Nicola     [  ]

Still need votes from @beldmit, @slontis, @mspncp and @romen.

t8m commented 2 years ago

Re size_t for all array indexes, I would say that consistency is a factor. It makes no sense to have different index types depending on the size of the array (what would be the pivot size?)

Hmm, I actually agree with Tim that size_t should be used only in case we do not know the index will be small. Pivot size? Anything that clearly fits into 31 bits of the guaranteed int size? But I would not make it hard requirement to use int as index for these smallish arrays.

t8m commented 2 years ago

Anyway I still think this is an improvement for start and can be adjusted further in further PRs.

romen commented 2 years ago

Vote: [+0]

beldmit commented 2 years ago

I'd like to see some amendments according to the discussion. I strongly like the approach as a whole. [+0]

mspncp commented 2 years ago

Vote: [0]

t8m commented 2 years ago

Now it's on @slontis to determine the outcome of the vote.

slontis commented 2 years ago

Vote: [-1] I think it probably goes too far - it needs to be simpler to pass.

mattcaswell commented 2 years ago

I have now closed this vote. The final result was:

Topic: Deprecate long and add notes on integer types
Proposed by: Matt Caswell
Issue link: https://github.com/openssl/technical-policies/pull/51
Public: yes
Opened: 2022-07-25
Closed: 2022-08-02
Accepted:  no  (for: 3, against: 3, abstained: 4, not voted: 0)

  Dmitry     [+0]
  Matt       [+1]
  Pauli      [-1]
  Tim        [-1]
  Richard    [ 0]
  Shane      [-1]
  Tomas      [+1]
  Kurt       [+1]
  Matthias   [ 0]
  Nicola     [+0]
mattcaswell commented 2 years ago

It seems to me that there would still be some support within OTC for some variation of this PR, so I will leave this open for now to enable OTC to determine next steps.