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

Jump to bottom

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

Closed
josharian opened this issue Sep 4, 2021 · 15 comments
Closed

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

josharian opened this issue Sep 4, 2021 · 15 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker umbrella
Milestone
Go1.20

Comments

@josharian
Copy link
Contributor

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

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 the NeedsFix The path to resolution is known, but the work has not been done. label Sep 4, 2021
@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

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

@griesemer
Copy link
Contributor

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

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

@mdempsky mdempsky modified the milestones: Go1.18, Go1.19 Sep 22, 2021
@danscales danscales removed their assignment Oct 7, 2021
@griesemer griesemer removed their assignment Feb 1, 2022
@aclements
Copy link
Member

There are a handful of unreviewed files remaining. Given that we're going to enable unified IR soon, can we get a quick status update on reviewing these?

internal/importer/ureader.go:1:// UNREVIEWED
internal/noder/codes.go:1:// UNREVIEWED
internal/noder/linker.go:1:// UNREVIEWED
internal/noder/quirks.go:1:// UNREVIEWED
internal/noder/reader.go:1:// UNREVIEWED
internal/noder/unified.go:1:// UNREVIEWED
internal/noder/writer.go:1:// UNREVIEWED

@griesemer
Copy link
Contributor

@aclements My understanding is that @mdempsky is working on getting all the remaining prerequisites in place (such as a compiler-matching importer for go/types, which I reviewed yesterday). We will review these files soon.

@gopherbot
Copy link

Change https://go.dev/cl/407614 mentions this issue: internal/pkgbits: finish documentation

@gopherbot
Copy link

Change https://go.dev/cl/408234 mentions this issue: cmd/compile: more Unified IR docs and review

gopherbot pushed a commit that referenced this issue May 25, 2022
@mdempsky
internal/pkgbits: finish documentation
8841699 
This CL adds documentation for all exported pkgbits APIs, and removes
its UNREVIEWED comments.

Updates #48194.

Change-Id: Ifff548cd9f31a5c5cc5f400a6dae5c98c46ec4ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/407614
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
@aclements
Copy link
Member

aclements commented May 25, 2022 •
edited

If I'm not mistaken, once CL 408234 lands, the only remaining unreviewed file should be cmd/compile/internal/importer/ureader.go

Given that we're not enabling unified IR for 1.19, it seems like it should be okay to mark this okay-after-beta1 (or possibly even drop release-blocker?)

(This is great progress!)

(Edited: I meant "1.19", not "1.20" above.)

@mdempsky mdempsky modified the milestones: Go1.19, Go1.20 May 25, 2022
@mdempsky
Copy link
Member

Yeah, we can bump the issue for 1.20.

@griesemer

This comment was marked as resolved.

Sign in to view
@gopherbot
Copy link

Change https://go.dev/cl/411917 mentions this issue: [dev.unified] cmd/compile: more Unified IR docs and review

gopherbot pushed a commit that referenced this issue Jun 14, 2022
@mdempsky @gopherbot
[dev.unified] cmd/compile: more Unified IR docs and review
394ea70 
This adds more documentation throughout the core Unified IR logic and
removes their UNREVIEWED notices.

Updates #48194.

Change-Id: Iddd30edaee1c6ea8a05a5a7e013480e02be00d29
Reviewed-on: https://go-review.googlesource.com/c/go/+/411917
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@mdempsky @jproberts
[dev.unified] cmd/compile: more Unified IR docs and review
1d41f0c 
This adds more documentation throughout the core Unified IR logic and
removes their UNREVIEWED notices.

Updates golang#48194.

Change-Id: Iddd30edaee1c6ea8a05a5a7e013480e02be00d29
Reviewed-on: https://go-review.googlesource.com/c/go/+/411917
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@aclements
Copy link
Member

Just checking in, it looks like cmd/compile/internal/importer/ureader.go is the only remaining UNREVIEWED file?

@gopherbot
Copy link

Change https://go.dev/cl/422617 mentions this issue: all: remove remaining UNREVIEWED files for Unified IR

@cuonglm
Copy link
Member

cuonglm commented Aug 10, 2022

Just checking in, it looks like cmd/compile/internal/importer/ureader.go is the only remaining UNREVIEWED file?

I think we just forgot to remove the UNREVIEWED header for those files, I sent https://go-review.googlesource.com/c/go/+/422617 for this.

@golang golang locked and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker umbrella
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
9 participants
@toothrot @mdempsky @josharian @danscales @dmitshur @aclements @cuonglm @griesemer @gopherbot