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: "UNREVIEWED" headers scattered around the tree #48194

Open
josharian opened this issue Sep 4, 2021 · 4 comments
Open

cmd/compile: "UNREVIEWED" headers scattered around the tree #48194

josharian opened this issue Sep 4, 2021 · 4 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Sep 4, 2021

A bunch of files in the tree start:

// UNREVIEWED

I assume this is left over from generics work. Can those all be removed now?

cc @ianlancetaylor @griesemer @mdempsky

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 4, 2021

Files marked as //UNREVIEWED need to be reviewed explicitly. I see (under src):

./cmd/compile/internal/importer/testdata/exports.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/issue25301.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/issue25596.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/versions/test.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/issue20046.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/a.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/b.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/p.go:// UNREVIEWED
./cmd/compile/internal/importer/testdata/issue15920.go:// UNREVIEWED
./cmd/compile/internal/noder/unified.go:// UNREVIEWED
./cmd/compile/internal/noder/decoder.go:// UNREVIEWED
./cmd/compile/internal/noder/sync.go:// UNREVIEWED
./cmd/compile/internal/noder/encoder.go:// UNREVIEWED
./cmd/compile/internal/noder/reader2.go:// UNREVIEWED
./cmd/compile/internal/noder/quirks.go:// UNREVIEWED
./cmd/compile/internal/noder/codes.go:// UNREVIEWED
./cmd/compile/internal/noder/writer.go:// UNREVIEWED
./cmd/compile/internal/noder/reloc.go:// UNREVIEWED
./cmd/compile/internal/noder/linker.go:// UNREVIEWED
./cmd/compile/internal/noder/reader.go:// UNREVIEWED

The importer test files should match the corresponding test files elsewhere and should be straight-forward. We review them by comparing them against existing reviewed files and pointing out the differences, if any.

The noder files are related to @mdempsky 's work on the unified IR and need to be reviewed in full. This is planned to happen by the end of October (before the freeze).

@griesemer griesemer added this to the Go1.18 milestone Sep 4, 2021
@griesemer griesemer changed the title all: "UNREVIEWED" headers scattered around the tree cmd/compile: "UNREVIEWED" headers scattered around the tree Sep 4, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 22, 2021

Checking in on this issue as it's labeled a release blocker for Go 1.18. Is there any update?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 22, 2021

Several of these files have been reviewed and now we have a shorter list remaining:

./cmd/compile/internal/noder/unified.go:// UNREVIEWED
./cmd/compile/internal/noder/decoder.go:// UNREVIEWED
./cmd/compile/internal/noder/sync.go:// UNREVIEWED
./cmd/compile/internal/noder/encoder.go:// UNREVIEWED
./cmd/compile/internal/noder/reader2.go:// UNREVIEWED
./cmd/compile/internal/noder/quirks.go:// UNREVIEWED
./cmd/compile/internal/noder/codes.go:// UNREVIEWED
./cmd/compile/internal/noder/writer.go:// UNREVIEWED
./cmd/compile/internal/noder/reloc.go:// UNREVIEWED
./cmd/compile/internal/noder/linker.go:// UNREVIEWED
./cmd/compile/internal/noder/reader.go:// UNREVIEWED

These are all in new compiler files with functionality that may not be enabled by default for Go1.18. So the urgency to have this reviewed soon is lower.

cc: @mdempsky

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 22, 2021

Yeah, all of those files are only used for GOEXPERIMENT=unified, which we've decided to defer to Go 1.19.

@mdempsky mdempsky removed this from the Go1.18 milestone Sep 22, 2021
@mdempsky mdempsky added this to the Go1.19 milestone Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants