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

cmd/compile: Incomplete refactoring after CL that introduced package types? #19949

Closed
ALTree opened this issue Apr 12, 2017 · 5 comments
Closed

Comments

@ALTree
Copy link
Member

ALTree commented Apr 12, 2017

Looking at the truncfltlit function in gc/const.go I see code like this:

switch t.Etype {
	case types.TFLOAT64:
		d := fv.Float64()
		fv.SetFloat64(d)

	case TFLOAT32:
		d := fv.Float32()
		fv.SetFloat64(d)
}

The fact that we're using TFLOAT64 from types and TFLOAT32 from gc makes me suspect the refactoring done for CL 39855 (cmd/compile: factor out Pkg, Sym, and Type into package types) was somehow incomplete.

cc @griesemer

@josharian
Copy link
Contributor

josharian commented Apr 12, 2017

It does look incomplete. Fortunately, it doesn't matter too much from a correctness perspective. From the beginning of cmd/compile/internal/gc/type.go:

// convenience constants
const (
	Txxx = types.Txxx

	TINT8    = types.TINT8
	TUINT8   = types.TUINT8
	TINT16   = types.TINT16
	TUINT16  = types.TUINT16
	TINT32   = types.TINT32
	TUINT32  = types.TUINT32
	TINT64   = types.TINT64
	TUINT64  = types.TUINT64
	TINT     = types.TINT
	TUINT    = types.TUINT
	TUINTPTR = types.TUINTPTR

	TCOMPLEX64  = types.TCOMPLEX64
	TCOMPLEX128 = types.TCOMPLEX128

	TFLOAT32 = types.TFLOAT32
	TFLOAT64 = types.TFLOAT64

@griesemer
Copy link
Contributor

Indeed - I can't even see the difference anymore... . The factoring is in progress, so feel free to clean up as you see fit.

@ALTree
Copy link
Member Author

ALTree commented Apr 12, 2017

Thanks. To be sure: "clean up" means switch to the types. constants when possible (for example, always when writing new code); and the long term goal is to delete the dups @josharian pointed out exist in gc?

@griesemer
Copy link
Contributor

I started switching to the types constants but everything became incredibly verbose. So for now I'd say use the local constants. The factoring as it is clearly doesn't improve the code, but it makes it more explicit what kind of dependencies exist. Ideally (long run) we'd like to use a more direct representation of each type (one struct per type, rather than a Type struct with extension), and make Type an interface (which is already used in some form by the ssa backend).

@ALTree
Copy link
Member Author

ALTree commented Apr 13, 2017

Thanks for the explanation.

I'm going to close this issue since a) I fixed the specific inconsistency in truncfltlit in another CL b) it's now clear that the goal of your CL was not to switch everything to the types. constants.

@ALTree ALTree closed this as completed Apr 13, 2017
@golang golang locked and limited conversation to collaborators Apr 13, 2018
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

4 participants