Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: add Value.CanInt, CanUint, CanFloat #47658

Closed
bradfitz opened this issue Aug 11, 2021 · 10 comments
Closed

reflect: add Value.CanInt, CanUint, CanFloat #47658

bradfitz opened this issue Aug 11, 2021 · 10 comments

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Aug 11, 2021

reflect already has:

  • Value.CanAddr / Value.Addr
  • Value.CanInterface / Value.Interface
  • Value.Int
  • Value.Uint
  • Value.Float

Those latter three are tedious to use because you need to write:

switch rv.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
    foo(rv.Int())
}

I've used up my life quota of typing reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64 (and always forgetting reflect.Uintptr in the corresponding usually-copy-pasted uint set) and would now prefer to write the proposed CanFoo bool methods:

switch {
case rv.CanInt() {
    foo(rv.Int())
case rv.CanUint() {
    foo(rv.Uint())
case rv.CanFloat() {
    foo(rv.Float())
 }

Oh, and might as well add CanComplex too for consistency.

@cespare
Copy link
Contributor

cespare commented Aug 11, 2021

Also more code that will Just Work when we add int128.

(crosses fingers)

@dsnet
Copy link
Member

dsnet commented Aug 11, 2021

Counter proposal: Perhaps we should add accessor methods on reflect.Kind instead? I feel like reflect.Value has enough methods.

func (k reflect.Kind) IsInt()
func (k reflect.Kind) IsUint()
func (k reflect.Kind) IsFloat()
func (k reflect.Kind) IsComplex()

@bradfitz
Copy link
Contributor Author

@cespare, if int128 is added, we can't make Value.Int start returning int128 instead of int64, so making CanInt report true for int128 would be misleading. Likewise, @dsnet, those are fine, but would misleading/insufficient if int128 were added. I think CanX paired with X is more explicit and matches Addr/CanAddr and Interface/CanInterface.

@renthraysk
Copy link

How about a new type for representing a set of Kinds? Something like...

type Kinds uint

// Has return true when k is a member of Kinds, false otherwise.
func (s Kinds) Has(k reflect.Kind) bool {
	return (1<<k)&s != 0
}

const (
	Int64Kinds  Kinds = 1<<reflect.Int | 1<<reflect.Int8 | 1<<reflect.Int16 | 1<<reflect.Int32 | 1<<reflect.Int64
	Uint64Kinds Kinds = 1<<reflect.Uint | 1<<reflect.Uint8 | 1<<reflect.Uint16 | 1<<reflect.Uint32 | 1<<reflect.Uint64 | 1<<reflect.Uintptr
)

If new types get added, just need to add new constants.

@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@dsnet
Copy link
Member

dsnet commented Sep 9, 2021

I still prefer methods declared on reflect.Kind rather than reflect.Value since it's seems broadly more useful. There are times where I have a reflect.Kind on hand and I also do logic similar to:

case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:

Having methods be declared on reflect.Value unfortunately doesn't help these cases.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

In the (unlikely? likely?) future where we have int128, Kind.IsInt() would probably return true, but Value.CanInt() would probably return false. It seems like we need the more precise Value.CanInt in this case for deciding whether we are able to call Value.Int.

@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: add Value.CanInt, CanUint, CanFloat reflect: add Value.CanInt, CanUint, CanFloat Sep 22, 2021
@rsc rsc modified the milestones: Proposal, Backlog Sep 22, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/352131 mentions this issue: reflect: add Value.{CanInt, CanUint, CanFloat, CanComplex}

@golang golang locked and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants