Skip to content

Conversation

@timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 7, 2021

  • added toSigned, toUnsigned via re-export
  • I renamed private toUnsigned to castToUnsigned since it actually does a cast

In future work, we can improve how nim doc treats re-exports, but until then nim doc still works (at the cost of an extra click) and the re-export is transparent for client code; this has been discussed elsewhere.

note

this would've been a plausible alternative implementation:

when true:
  proc toUnsignedImpl(T: typedesc[SomeInteger]): auto =
    when T is int8: default(uint8)
    elif T is int16: default(uint16)
    elif T is int32: default(uint32)
    elif T is int64: default(uint64)
    elif T is int: default(uint)
    else: default(T)

  proc toSignedImpl(T: typedesc[SomeInteger]): auto =
    when T is uint8: default(int8)
    elif T is uint16: default(int16)
    elif T is uint32: default(int32)
    elif T is uint64: default(int64)
    elif T is uint: default(int)
    else: default(T)

  template toUnsigned(T: typedesc): untyped = typeof(toUnsignedImpl(T))
  template toSigned(T: typedesc): untyped = typeof(toSignedImpl(T))

with pros: uses generic caching instead of having to evaluate the when .. clauses at each call site at CT
cons: adds auxiliary symbol (seems minor con)

links

@timotheecour timotheecour marked this pull request as ready for review July 7, 2021 09:46
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 7, 2021
changelog.md Outdated
Added `HoleyEnum` for enums with holes, `OrdinalEnum` for enums without holes.
Added `hasClosure`.
Added `pointerBase` to return `T` for `ref T | ptr T`.
Added `toSigned`, `toUnsigned`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the RFC? Who asked for these API extensions? We cannot simply add everything that is occasionally useful...

@Araq
Copy link
Member

Araq commented Jul 21, 2021

D seems to lack it too: https://dlang.org/spec/traits.html

Is that enough of a reason not to include it? I mean, if it's the other way round and D offers something, we always need it too, right...

@Araq Araq closed this Jul 21, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Jul 21, 2021

D seems to lack it too: dlang.org/spec/traits.html

that's not true.

import std.traits
static assert(is(Unsigned!(int) == uint));
alias S1 = Signed!uint;
static assert(is(S1 == int));

this API is totally reasonable to expect in typetraits.

C++ also has it: https://en.cppreference.com/w/cpp/types/make_signed etc

@Araq
Copy link
Member

Araq commented Jul 22, 2021

Nevertheless we need some rules for what to add to the stdlib. "X is useful and other languages also have it" is not good enough, it would be a superset of everything some other language has. "X is useful, we have used it ourselves in this place" is not good enough either, the API surface of Nim keeps growing and edge cases we don't have to have spec'ed out need to spec'ed out before it can become a public API. (For example, is toUnsigned(range[0..7]) == range[0u..7u]?)

@timotheecour
Copy link
Member Author

timotheecour commented Jul 22, 2021

edge cases we don't have to have spec'ed out need to spec'ed out before it can become a public API. (For example, is toUnsigned(range[0..7]) == range[0u..7u]?)

see last commit, i'm for now disallowing range because it's unclear what to do with toSigned(range[0'u8..129'u8]) or toUnSigned(range[-1'i8..3'i8]); these edge cases can be supported in future if there's a need.

It's a useful API, it's not like the implementation will have to keep changing (apart from maybe adding support for range), and it's the logical module where one would expect to find this.

@Varriount
Copy link
Contributor

Varriount commented Jul 22, 2021

It's a useful API

Lots of things are useful. That doesn't necessarily mean they should be added to the standard library. For these procedures in particular, I can't actually envision any real-world scenarios where they would be used.

The situations where unsigned integers are typically most used are when interfacing with C, or when performing cryptographic/bit-manipulating operations. In both cases, one typically knows the data types they are converting both from and to.

If someone does run into scenarios where these kinds of conversions are required often enough to warrant encapsulation into a procedure, the logic isn't difficult to write.

In my view, functionality should only be added to the standard library if it is needed often enough, and is above a certain complexity threshold. The degree to which such functionality is already "tested" (such as if an external module were to be incorporated into the standard library) also plays a factor. Special circumstances notwithstanding, it's only when the sum of these factors is large enough, that incorporation into the standard library should be considered.

@timotheecour
Copy link
Member Author

I can't actually envision any real-world scenarios where they would be used.
The situations where unsigned integers are typically most used are when interfacing with C

what? no, unsigned ints have use cases beyond C interop.
eg, #18596 could replace this:

  when sizeof(T) == 8:
    type UT = uint64
  elif sizeof(T) == 4:
    type UT = uint32
  elif sizeof(T) == 2:
    type UT = uint16
  else:
    type UT = uint8

by this: type UT = toUnsigned(T)

likewise, bitops makes use of this functionality, and bitops is used in a number of places.

result = hasClosureImpl(fn)

from std/private/bitops_utils import toUnsigned, toSigned
export toUnsigned, toSigned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a "type trait", it seems wiser to publish "bitops_utils" instead, somehow.

Copy link
Member Author

@timotheecour timotheecour Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D defines those APIs in std.traits
C++ defines in std::type_traits
and we already have APIs like distinctBase, elementType in std/typetraits; it's much simpler to have these APIs (type => type) in std/typetraits than to create a dedicated bitops_utils module or equivalent (we already have bitops, and toUnsigned/toSigned is a poor fit for bitops) whose scope would be unclear.

type traits don't just cover boolean properties of types, as evidenced both by what std/typetraits already contains, and by descriptions of C++ type traits, eg:

https://www.internalpointers.com/post/quick-primer-type-traits-modern-cpp

Type traits can also apply some transformation to a type. For example, given T, you can add/remove the const specifier, the reference or the pointer, or yet turn it into a signed/unsigned type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move them to typetraits directly then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@timotheecour
Copy link
Member Author

PTAL

@timotheecour
Copy link
Member Author

ping @Araq

template toUnsigned*(x: int64): uint64 = cast[uint64](x)
template toUnsigned*(x: int): uint = cast[uint](x)
template toUnsigned(T: typedesc[SomeInteger and not range]): untyped =
# copied/adapted from std/typetraits.toUnsigned to avoid a `system => typetraits` dependency
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you cannot use typetraits here, there is no need to copy&paste, you can keep the old version of bitops_utils.nim as it's at least shorter.

Copy link
Member Author

@timotheecour timotheecour Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, mostly; kept the name castToUnsigned as toUnsigned is misleading (it casts instead of doing a conversion in traditional sense); also the way I'm doing it allows simplifying all call sites by avoiding to branch on whether the input is signed vs unsigned (as evidenced by the diff)

@timotheecour
Copy link
Member Author

PTAL

@Araq Araq added merge_when_passes_CI mergeable once green and removed Ready For Review (please take another look): ready for next review round labels Mar 24, 2022
@ringabout ringabout closed this Apr 5, 2022
@ringabout ringabout reopened this Apr 5, 2022
@Varriount Varriount merged commit e78ef57 into nim-lang:devel Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge_when_passes_CI mergeable once green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants