go/build: clean up ctxt.shouldBuild, tests

Make ctxt.shouldBuild return multiple values
instead of modifying *sawBinaryOnly in place.
Also give it a table-driven test.

Cleanup in preparation for boolean expressions,
but nice even if those don't end up happening.

Change-Id: Ibb78b0080070deafac7299a6de87ab8bebeb702d
Reviewed-on: https://go-review.googlesource.com/c/go/+/240598
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/go/build/build.go

@@ -1371,8 +1371,8 @@ func (ctxt *Context) matchFile(dir, name string, allTags map[string]bool, binary
 	}
 
 	// Look for +build comments to accept or reject the file.
-	var sawBinaryOnly bool
+	ok, sawBinaryOnly := ctxt.shouldBuild(data, allTags)
-	if !ctxt.shouldBuild(data, allTags, &sawBinaryOnly) && !ctxt.UseAllFiles {
+	if !ok && !ctxt.UseAllFiles {
 		return
 	}
 
@@ -1422,10 +1422,11 @@ var binaryOnlyComment = []byte("//go:binary-only-package")
 //
 // marks the file as applicable only on Windows and Linux.
 //
-// If shouldBuild finds a //go:binary-only-package comment in the file,
+// For each build tag it consults, shouldBuild sets allTags[tag] = true.
-// it sets *binaryOnly to true. Otherwise it does not change *binaryOnly.
 //
-func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool, binaryOnly *bool) bool {
+// shouldBuild reports whether the file should be built
+// and whether a //go:binary-only-package comment was found.
+func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shouldBuild bool, binaryOnly bool) {
 	sawBinaryOnly := false
 
 	// Pass 1. Identify leading run of // comments and blank lines,
@@ -1452,7 +1453,7 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool, binary
 
 	// Pass 2.  Process each line in the run.
 	p = content
-	allok := true
+	shouldBuild = true
 	for len(p) > 0 {
 		line := p
 		if i := bytes.IndexByte(line, '\n'); i >= 0 {
@@ -1479,17 +1480,13 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool, binary
 					}
 				}
 				if !ok {
-					allok = false
+					shouldBuild = false
 				}
 			}
 		}
 	}
 
-	if binaryOnly != nil && sawBinaryOnly {
+	return shouldBuild, sawBinaryOnly
-		*binaryOnly = true
-	}
-
-	return allok
 }
 
 // saveCgo saves the information from the #cgo lines in the import "C" comment.

src/go/build/build_test.go

@@ -6,6 +6,7 @@ package build
 
 import (
 	"flag"
+	"fmt"
 	"internal/testenv"
 	"io"
 	"io/ioutil"
@@ -138,48 +139,78 @@ func TestLocalDirectory(t *testing.T) {
 	}
 }
 
-func TestShouldBuild(t *testing.T) {
+var shouldBuildTests = []struct {
-	const file1 = "// +build tag1\n\n" +
+	content     string
-		"package main\n"
+	tags        map[string]bool
-	want1 := map[string]bool{"tag1": true}
+	binaryOnly  bool
-
+	shouldBuild bool
-	const file2 = "// +build cgo\n\n" +
+}{
+	{
+		content: "// +build yes\n\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true},
+		shouldBuild: true,
+	},
+	{
+		content: "// +build no yes\n\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true, "no": true},
+		shouldBuild: true,
+	},
+	{
+		content: "// +build no,yes no\n\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true, "no": true},
+		shouldBuild: false,
+	},
+	{
+		content: "// +build cgo\n\n" +
+			"// Copyright The Go Authors.\n\n" +
 			"// This package implements parsing of tags like\n" +
 			"// +build tag1\n" +
-		"package build"
+			"package build",
-	want2 := map[string]bool{"cgo": true}
+		tags:        map[string]bool{"cgo": true},
-
+		shouldBuild: false,
-	const file3 = "// Copyright The Go Authors.\n\n" +
+	},
+	{
+		content: "// Copyright The Go Authors.\n\n" +
 			"package build\n\n" +
 			"// shouldBuild checks tags given by lines of the form\n" +
 			"// +build tag\n" +
-		"func shouldBuild(content []byte)\n"
+			"func shouldBuild(content []byte)\n",
-	want3 := map[string]bool{}
+		tags:        map[string]bool{},
+		shouldBuild: true,
+	},
+	{
+		// too close to package line
+		content: "// +build yes\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: true,
+	},
+	{
+		// too close to package line
+		content: "// +build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: true,
+	},
+}
 
-	ctx := &Context{BuildTags: []string{"tag1"}}
+func TestShouldBuild(t *testing.T) {
-	m := map[string]bool{}
+	for i, tt := range shouldBuildTests {
-	if !ctx.shouldBuild([]byte(file1), m, nil) {
+		t.Run(fmt.Sprint(i), func(t *testing.T) {
-		t.Errorf("shouldBuild(file1) = false, want true")
+			ctx := &Context{BuildTags: []string{"yes"}}
+			tags := map[string]bool{}
+			shouldBuild, binaryOnly := ctx.shouldBuild([]byte(tt.content), tags)
+			if shouldBuild != tt.shouldBuild || binaryOnly != tt.binaryOnly || !reflect.DeepEqual(tags, tt.tags) {
+				t.Errorf("mismatch:\n"+
+					"have shouldBuild=%v, binaryOnly=%v, tags=%v\n"+
+					"want shouldBuild=%v, binaryOnly=%v, tags=%v",
+					shouldBuild, binaryOnly, tags,
+					tt.shouldBuild, tt.binaryOnly, tt.tags)
 			}
-	if !reflect.DeepEqual(m, want1) {
+		})
-		t.Errorf("shouldBuild(file1) tags = %v, want %v", m, want1)
-	}
-
-	m = map[string]bool{}
-	if ctx.shouldBuild([]byte(file2), m, nil) {
-		t.Errorf("shouldBuild(file2) = true, want false")
-	}
-	if !reflect.DeepEqual(m, want2) {
-		t.Errorf("shouldBuild(file2) tags = %v, want %v", m, want2)
-	}
-
-	m = map[string]bool{}
-	ctx = &Context{BuildTags: nil}
-	if !ctx.shouldBuild([]byte(file3), m, nil) {
-		t.Errorf("shouldBuild(file3) = false, want true")
-	}
-	if !reflect.DeepEqual(m, want3) {
-		t.Errorf("shouldBuild(file3) tags = %v, want %v", m, want3)
 	}
 }