ronantakizawa/codereview-bench huggingface dataset:
Repository: syncthing/syncthing
File: lib/beacon/beacon.go (Go)

Code before:
			// Don't retry too frenetically: an error to open a socket or
			// whatever is usually something that is either permanent or takes
			// a while to get solved...
			FailureThreshold: 2,
			FailureBackoff:   60 * time.Second,
			// Only log restarts in debug mode.
			Log: func(line string) {
				l.Debugln(line)
			},
			PassThroughPanics: true,
		}),
		name:    name,
		inbox:   make(chan []byte),
		outbox:  make(chan recv, 16),
		stopped: make(chan struct{}),
	}
}

func (c *cast) addReader(r util.ServiceWithError) {
	c.reader = r
	c.Add(r)
}

func (c *cast) addWriter(w util.ServiceWithError) {
	c.writer = w
	c.Add(w)
}

func (c *cast) Stop() {
	c.Supervisor.Stop()
	close(c.stopped)
}

func (c *cast) String() string {
	return fmt.Sprintf("%s@%p", c.name, c)
}

func (c *cast) Send(data []byte) {
	select {
	case c.inbox <- data:
	case <-c.stopped:
	}
}

func (c *cast) Recv() ([]byte, net.Addr) {
	select {
	case recv := <-c.outbox:
		return recv.data, recv.src
	case <-c.stopped:
	}
	return nil, nil

Review comment (bug):
This could just be:
```
func (c *cast) addWriter(svc func(stop chan struct{}) error) {
    c.writer = util.ServiceWithError(func(stop chan struct) error {
         l.Debug("Starting " + c.name + " writer")
         err := svc(stop)
         l.Debug("Stopped " + c.name + " writer", err)
         return err
    })
    c.Add(c.writer)
}
```

then on both services you just do:
```
        c.addWriter(func(stop chan struct{}) error {
		return readBroadcasts(c.outbox, port, stop)
	})
```


Also, note that reader and writer are the other way around?

Code after:
			// Don't retry too frenetically: an error to open a socket or
			// whatever is usually something that is either permanent or takes
			// a while to get solved...
			FailureThreshold: 2,
			FailureBackoff:   60 * time.Second,
			// Only log restarts in debug mode.
			Log: func(line string) {
				l.Debugln(line)
			},
			PassThroughPanics: true,
		}),
		name:    name,
		inbox:   make(chan []byte),
		outbox:  make(chan recv, 16),
		stopped: make(chan struct{}),
	}
}

func (c *cast) addReader(svc func(chan struct{}) error) {
	c.reader = c.createService(svc, "reader")
	c.Add(c.reader)
}

func (c *cast) addWriter(svc func(stop chan struct{}) error) {
	c.writer = c.createService(svc, "writer")
	c.Add(c.writer)
}

func (c *cast) createService(svc func(chan struct{}) error, suffix string) util.ServiceWithError {
	return util.AsServiceWithError(func(stop chan struct{}) error {
		l.Debugln("Starting", c.name, suffix)
		err := svc(stop)
		l.Debugf("Stopped %v %v: %v", c.name, suffix, err)
		return err
	})
}

func (c *cast) Stop() {
	c.Supervisor.Stop()
	close(c.stopped)
}

func (c *cast) String() string {
	return fmt.Sprintf("%s@%p", c.name, c)
}

func (c *cast) Send(data []byte) {
	select {
	case c.inbox <- data:
	case <-c.stopped:
	}
Repository: syncthing/syncthing
File: lib/beacon/broadcast.go (Go)

Code before:
// Copyright (C) 2014 The Syncthing Authors.
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

package beacon

import (
	"net"
	"time"

	"github.com/syncthing/syncthing/lib/util"
)

func NewBroadcast(port int) Interface {
	c := newCast("broadcastBeacon")
	c.addReader(util.AsServiceWithError(func(stop chan struct{}) error {
		return writeBroadcasts(c.inbox, port, stop)
	}))
	c.addWriter(util.AsServiceWithError(func(stop chan struct{}) error {
		return readBroadcasts(c.outbox, port, stop)
	}))
	return c
}

func writeBroadcasts(inbox <-chan []byte, port int, stop chan struct{}) error {
	l.Debugln("starting writeBroadcasts")
	defer l.Debugln("stopping writeBroadcasts")

	conn, err := net.ListenUDP("udp4", nil)
	if err != nil {
		l.Debugln(err)
		return err
	}
	done := make(chan struct{})
	defer close(done)
	go func() {
		select {
		case <-stop:
		case <-done:
		}
		conn.Close()
	}()

	for {

Review comment (suggestion):
Side note, not related.
AsServiceWithError could use one stackframe or two stackframes above as the service name.

Code after:
// Copyright (C) 2014 The Syncthing Authors.
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

package beacon

import (
	"net"
	"time"
)

func NewBroadcast(port int) Interface {
	c := newCast("broadcastBeacon")
	c.addReader(func(stop chan struct{}) error {
		return readBroadcasts(c.outbox, port, stop)
	})
	c.addWriter(func(stop chan struct{}) error {
		return writeBroadcasts(c.inbox, port, stop)
	})
	return c
}

func writeBroadcasts(inbox <-chan []byte, port int, stop chan struct{}) error {
	conn, err := net.ListenUDP("udp4", nil)
	if err != nil {
		l.Debugln(err)
		return err
	}
	done := make(chan struct{})
	defer close(done)
	go func() {
		select {
		case <-stop:
		case <-done:
		}
		conn.Close()
	}()

	for {
		var bs []byte
		select {
		case bs = <-inbox:
		case <-stop:
			return nil
Repository: syncthing/syncthing
File: lib/db/lowlevel.go (Go)

Code before:
package db

import (
	"os"
	"strconv"
	"strings"
	"sync"
	"sync/atomic"

	"github.com/syndtr/goleveldb/leveldb"
	"github.com/syndtr/goleveldb/leveldb/errors"
	"github.com/syndtr/goleveldb/leveldb/iterator"
	"github.com/syndtr/goleveldb/leveldb/opt"
	"github.com/syndtr/goleveldb/leveldb/storage"
	"github.com/syndtr/goleveldb/leveldb/util"
)

const (
	dbMaxOpenFiles = 100
	dbFlushBatch   = 4 << MiB

	// A large database is > 200 MiB. It's a mostly arbitrary value, but
	// it's also the case that each file is 2 MiB by default and when we
	// have dbMaxOpenFiles of them we will need to start thrashing fd:s.
	// Switching to large database settings causes larger files to be used
	// when compaticting, reducing the number.
	largeDBThreshold = dbMaxOpenFiles * (2 << MiB)

	KiB = 10
	MiB = 20
)

// Lowlevel is the lowest level database interface. It has a very simple
// purpose: hold the actual *leveldb.DB database, and the in-memory state
// that belong to that database. In the same way that a single on disk
// database can only be opened once, there should be only one Lowlevel for
// any given *leveldb.DB.
type Lowlevel struct {
	committed int64 // atomic, must come first
	*leveldb.DB
	location  string
	folderIdx *smallIndex
	deviceIdx *smallIndex
	closed    bool
	closeMut  *sync.RWMutex
	iterWG    sync.WaitGroup
}

// Open attempts to open the database at the given location, and runs
// recovery on it if opening fails. Worst case, if recovery is not possible,
// the database is erased and created from scratch.

Review comment (suggestion):
```suggestion
	// when compacting, reducing the number.
```

Code after:
package db

import (
	"os"
	"strconv"
	"strings"
	"sync"
	"sync/atomic"

	"github.com/syndtr/goleveldb/leveldb"
	"github.com/syndtr/goleveldb/leveldb/errors"
	"github.com/syndtr/goleveldb/leveldb/iterator"
	"github.com/syndtr/goleveldb/leveldb/opt"
	"github.com/syndtr/goleveldb/leveldb/storage"
	"github.com/syndtr/goleveldb/leveldb/util"
)

const (
	dbMaxOpenFiles = 100
	dbFlushBatch   = 4 << MiB

	// A large database is > 200 MiB. It's a mostly arbitrary value, but
	// it's also the case that each file is 2 MiB by default and when we
	// have dbMaxOpenFiles of them we will need to start thrashing fd:s.
	// Switching to large database settings causes larger files to be used
	// when compacting, reducing the number.
	dbLargeThreshold = dbMaxOpenFiles * (2 << MiB)

	KiB = 10
	MiB = 20
)

type Tuning int

const (
	// N.b. these constants must match those in lib/config.Tuning!
	TuningAuto Tuning = iota
	TuningSmall
	TuningLarge
)

// Lowlevel is the lowest level database interface. It has a very simple
// purpose: hold the actual *leveldb.DB database, and the in-memory state
// that belong to that database. In the same way that a single on disk
// database can only be opened once, there should be only one Lowlevel for
// any given *leveldb.DB.
type Lowlevel struct {
	committed int64 // atomic, must come first
	*leveldb.DB
	location  string
	folderIdx *smallIndex
Repository: syncthing/syncthing
File: lib/connections/quic_listen.go (Go)

Code before:
	go svc.Serve()
	defer svc.Stop()

	registry.Register(t.uri.Scheme, conn)
	defer registry.Unregister(t.uri.Scheme, conn)

	listener, err := quic.Listen(conn, t.tlsCfg, quicConfig)
	if err != nil {
		l.Infoln("Listen (BEP/quic):", err)
		return err
	}
	defer listener.Close()

	l.Infof("QUIC listener (%v) starting", packetConn.LocalAddr())
	defer l.Infof("QUIC listener (%v) shutting down", packetConn.LocalAddr())

	for {
		select {
		case <-ctx.Done():
			return nil
		default:
		}

		session, err := listener.Accept(ctx)
		if err != nil {
			if err, ok := err.(net.Error); !ok || !err.Timeout() {
				l.Warnln("Listen (BEP/quic): Accepting connection:", err)
			}
			continue
		}
		l.Debugln("connect from", session.RemoteAddr())

		streamCtx, cancel := context.WithTimeout(ctx, quicOperationTimeout)
		stream, err := session.AcceptStream(streamCtx)
		cancel()
		if err != nil {
			l.Debugf("failed to accept stream from %s: %v", session.RemoteAddr(), err)
			_ = session.Close()
			continue
		}

		t.conns <- internalConn{&quicTlsConn{session, stream, nil}, connTypeQUICServer, quicPriority}
	}
}

func (t *quicListener) URI() *url.URL {
	return t.uri
}

func (t *quicListener) WANAddresses() []*url.URL {
	uris := t.LANAddresses()

Review comment (suggestion):
Also don't warn for `err == context.Canceled`. And in principle that could be used to break the loop instead of the select above (but I think the explicit check at the beginning like now is cleaner).

Code after:
	go svc.Serve()
	defer svc.Stop()

	registry.Register(t.uri.Scheme, conn)
	defer registry.Unregister(t.uri.Scheme, conn)

	listener, err := quic.Listen(conn, t.tlsCfg, quicConfig)
	if err != nil {
		l.Infoln("Listen (BEP/quic):", err)
		return err
	}
	defer listener.Close()

	l.Infof("QUIC listener (%v) starting", packetConn.LocalAddr())
	defer l.Infof("QUIC listener (%v) shutting down", packetConn.LocalAddr())

	for {
		select {
		case <-ctx.Done():
			return nil
		default:
		}

		session, err := listener.Accept(ctx)
		if err == context.Canceled {
			return nil
		} else if err != nil {
			l.Warnln("Listen (BEP/quic): Accepting connection:", err)
			continue
		}
		l.Debugln("connect from", session.RemoteAddr())

		streamCtx, cancel := context.WithTimeout(ctx, quicOperationTimeout)
		stream, err := session.AcceptStream(streamCtx)
		cancel()
		if err != nil {
			l.Debugf("failed to accept stream from %s: %v", session.RemoteAddr(), err)
			_ = session.Close()
			continue
		}

		t.conns <- internalConn{&quicTlsConn{session, stream, nil}, connTypeQUICServer, quicPriority}
	}
}

func (t *quicListener) URI() *url.URL {
	return t.uri
}

func (t *quicListener) WANAddresses() []*url.URL {
	uris := t.LANAddresses()
Repository: syncthing/syncthing
File: lib/build/build.go (Go)

Code before:
	"strings"
	"time"
)

var (
	// Injected by build script
	Version = "unknown-dev"
	Host    = "unknown" // Set by build script
	User    = "unknown" // Set by build script
	Stamp   = "0"       // Set by build script

	// Static
	Codename = "Fermium Flea"

	// Set by init()
	Date        time.Time
	IsRelease   bool
	IsCandidate bool
	IsBeta      bool
	LongVersion string

	// Set by Go build tags
	Tags []string

	// Set by the caller
	program = "syncthing"

	allowedVersionExp = regexp.MustCompile(`^v\d+\.\d+\.\d+(-[a-z0-9]+)*(\.\d+)*(\+\d+-g[0-9a-f]+)?(-[^\s]+)?$`)
)

func init() {
	if Version != "unknown-dev" {
		// If not a generic dev build, version string should come from git describe
		if !allowedVersionExp.MatchString(Version) {
			log.Fatalf("Invalid version string %q;\n\tdoes not match regexp %v", Version, allowedVersionExp)
		}
	}
	setBuildData()
	setLongVersion()
}

func SetProgram(name string) {
	program = name
	setLongVersion()
}

func setBuildData() {
	// Check for a clean release build. A release is something like
	// "v0.1.2", with an optional suffix of letters and dot separated
	// numbers like "-beta3.47". If there's more stuff, like a plus sign and
	// a commit hash and so on, then it's not a release. If it has a dash in

Review comment (suggestion):
I guess using `Syncthing` for the proper "brand name" would be too scary (as there might be places where `syncthing` is expected/hardcoded).

Code after:
	"strings"
	"time"
)

var (
	// Injected by build script
	Program = "syncthing"
	Version = "unknown-dev"
	Host    = "unknown"
	User    = "unknown"
	Stamp   = "0"

	// Static
	Codename = "Fermium Flea"

	// Set by init()
	Date        time.Time
	IsRelease   bool
	IsCandidate bool
	IsBeta      bool
	LongVersion string

	// Set by Go build tags
	Tags []string

	allowedVersionExp = regexp.MustCompile(`^v\d+\.\d+\.\d+(-[a-z0-9]+)*(\.\d+)*(\+\d+-g[0-9a-f]+)?(-[^\s]+)?$`)
)

func init() {
	if Version != "unknown-dev" {
		// If not a generic dev build, version string should come from git describe
		if !allowedVersionExp.MatchString(Version) {
			log.Fatalf("Invalid version string %q;\n\tdoes not match regexp %v", Version, allowedVersionExp)
		}
	}
	setBuildData()
}

func setBuildData() {
	// Check for a clean release build. A release is something like
	// "v0.1.2", with an optional suffix of letters and dot separated
	// numbers like "-beta3.47". If there's more stuff, like a plus sign and
	// a commit hash and so on, then it's not a release. If it has a dash in
	// it, it's some sort of beta, release candidate or special build. If it
	// has "-rc." in it, like "v0.14.35-rc.42", then it's a candidate build.
	//
	// So, every build that is not a stable release build has IsBeta = true.
	// This is used to enable some extra debugging (the deadlock detector).
	//
	// Release candidate builds are also "betas" from this point of view and
	// will have that debugging enabled. In addition, some features are
Repository: syncthing/syncthing
File: cmd/stdiscosrv/apisrv.go (Go)

Code before:
			}
		}
	} else if hdr := req.Header.Get("X-Forwarded-Tls-Client-Cert"); hdr != "" {
		// Traefik 2 passtlsclientcert
		// The certificate is in PEM format with url encoding but without newlines
		// and start/end statements. We need to decode, reinstate the newlines every 64
		// character and add statements for the PEM decoder
		hdr, err := url.QueryUnescape(hdr)
		if err != nil {
			log.Fatal(err)
			return nil
		}

		for i := 64; i < len(hdr); i += 65 {
			hdr = hdr[:i] + "\n" + hdr[i:]
		}

		hdr = "-----BEGIN CERTIFICATE-----\n" + hdr
		hdr = hdr + "\n-----END CERTIFICATE-----\n"
		bs = []byte(hdr)
	}

	block, _ := pem.Decode(bs)
	if block == nil {
		// Decoding failed
		log.Println("Decoding failed")
		return nil
	}

	return block.Bytes
}

// fixupAddresses checks the list of addresses, removing invalid ones and
// replacing unspecified IPs with the given remote IP.
func fixupAddresses(remote net.IP, addresses []string) []string {
	fixed := make([]string, 0, len(addresses))
	for _, annAddr := range addresses {
		uri, err := url.Parse(annAddr)
		if err != nil {
			continue
		}

		host, port, err := net.SplitHostPort(uri.Host)
		if err != nil {
			continue
		}

		ip := net.ParseIP(host)

		// Some classes of IP are no-go.
		if ip.IsLoopback() || ip.IsMulticast() {

Review comment (suggestion):
This is not a very meaningful error, we should not log it.

Code after:
			}
		}
	} else if hdr := req.Header.Get("X-Forwarded-Tls-Client-Cert"); hdr != "" {
		// Traefik 2 passtlsclientcert
		// The certificate is in PEM format with url encoding but without newlines
		// and start/end statements. We need to decode, reinstate the newlines every 64
		// character and add statements for the PEM decoder
		hdr, err := url.QueryUnescape(hdr)
		if err != nil {
			// Decoding failed
			return nil
		}

		for i := 64; i < len(hdr); i += 65 {
			hdr = hdr[:i] + "\n" + hdr[i:]
		}

		hdr = "-----BEGIN CERTIFICATE-----\n" + hdr
		hdr = hdr + "\n-----END CERTIFICATE-----\n"
		bs = []byte(hdr)
	}

	if bs == nil {
		return nil
	}

	block, _ := pem.Decode(bs)
	if block == nil {
		// Decoding failed
		return nil
	}

	return block.Bytes
}

// fixupAddresses checks the list of addresses, removing invalid ones and
// replacing unspecified IPs with the given remote IP.
func fixupAddresses(remote net.IP, addresses []string) []string {
	fixed := make([]string, 0, len(addresses))
	for _, annAddr := range addresses {
		uri, err := url.Parse(annAddr)
		if err != nil {
			continue
		}

		host, port, err := net.SplitHostPort(uri.Host)
		if err != nil {
			continue
		}

		ip := net.ParseIP(host)
Repository: syncthing/syncthing
File: lib/protocol/bufferpool_test.go (Go)

Code before:
package protocol

import "testing"

func TestBucketNumbers(t *testing.T) {
	cases := []struct {
		size int
		put  int
		get  int
	}{
		// Small blocks are put nowwhere, but can be fetched from bucket 0
		{size: 1024, put: -1, get: 0},

		// The blocksize for bucket zero  goes there
		{size: MinBlockSize, put: 0, get: 0},

		// Up to the next blocksize - 1 we still put in the same bucket, but
		// we look for these blocks in the next bucket where we know they
		// can be found.
		{size: 2*MinBlockSize - 1, put: 0, get: 1},

		// Next blocksize at the border...
		{size: 2 * MinBlockSize, put: 1, get: 1},

		// ... and past it.
		{size: 2*MinBlockSize + 1, put: 1, get: 2},
	}

	for _, tc := range cases {
		getRes := getBucketForSize(tc.size)
		if getRes != tc.get {
			t.Errorf("block of size %d should get from bucket %d, not %d", tc.size, tc.get, getRes)
		}
		putRes := putBucketForSize(tc.size)
		if putRes != tc.put {
			t.Errorf("block of size %d should put into bucket %d, not %d", tc.size, tc.put, putRes)
		}
	}
}


Review comment (suggestion):
I don't get this. We should always allocate bucket sized slices, and we should always get and return them to the same pool.

Code after:
package protocol

import (
	"sync"
	"testing"
	"time"

	"github.com/syncthing/syncthing/lib/rand"
)

func TestGetBucketNumbers(t *testing.T) {
	cases := []struct {
		size   int
		bkt    int
		panics bool
	}{
		{size: 1024, bkt: 0},
		{size: MinBlockSize, bkt: 0},
		{size: MinBlockSize + 1, bkt: 1},
		{size: 2*MinBlockSize - 1, bkt: 1},
		{size: 2 * MinBlockSize, bkt: 1},
		{size: 2*MinBlockSize + 1, bkt: 2},
		{size: MaxBlockSize, bkt: len(BlockSizes) - 1},
		{size: MaxBlockSize + 1, panics: true},
	}

	for _, tc := range cases {
		if tc.panics {
			shouldPanic(t, func() { getBucketForLen(tc.size) })
		} else {
			res := getBucketForLen(tc.size)
			if res != tc.bkt {
				t.Errorf("block of size %d should get from bucket %d, not %d", tc.size, tc.bkt, res)
			}
		}
	}
}

func TestPutBucketNumbers(t *testing.T) {
	cases := []struct {
		size   int
		bkt    int
		panics bool
	}{
		{size: 1024, panics: true},
		{size: MinBlockSize, bkt: 0},
		{size: MinBlockSize + 1, panics: true},
		{size: 2 * MinBlockSize, bkt: 1},
		{size: MaxBlockSize, bkt: len(BlockSizes) - 1},
		{size: MaxBlockSize + 1, panics: true},
	}
Repository: syncthing/syncthing
File: cmd/syncthing/main.go (Go)

Code before:
	"path/filepath"
	"runtime"
	"runtime/pprof"
	"sort"
	"strconv"
	"syscall"
	"time"

	"github.com/syncthing/syncthing/lib/build"
	"github.com/syncthing/syncthing/lib/config"
	"github.com/syncthing/syncthing/lib/dialer"
	"github.com/syncthing/syncthing/lib/events"
	"github.com/syncthing/syncthing/lib/fs"
	"github.com/syncthing/syncthing/lib/locations"
	"github.com/syncthing/syncthing/lib/logger"
	"github.com/syncthing/syncthing/lib/osutil"
	"github.com/syncthing/syncthing/lib/protocol"
	"github.com/syncthing/syncthing/lib/syncthing"
	"github.com/syncthing/syncthing/lib/tlsutil"
	"github.com/syncthing/syncthing/lib/upgrade"

	"github.com/pkg/errors"
)

const (
	exitError              = 1
	exitNoUpgradeAvailable = 2
	exitUpgrading          = 4
)

const (
	tlsDefaultCommonName   = "syncthing"
	deviceCertLifetimeDays = 20 * 365
)

const (
	usage      = "syncthing [options]"
	extraUsage = `
The -logflags value is a sum of the following:

   1  Date
   2  Time
   4  Microsecond time
   8  Long filename
  16  Short filename

I.e. to prefix each log line with date and time, set -logflags=3 (1 + 2 from
above). The value 0 is used to disable all of the above. The default is to
show time only (2).



Review comment (suggestion):
Should probably use [these](https://github.com/syncthing/syncthing/blob/31569debeb56d7e11b3d02577157c520ef0236a7/lib/syncthing/syncthing.go#L50-L55) instead for all of these

Code after:
	"path/filepath"
	"runtime"
	"runtime/pprof"
	"sort"
	"strconv"
	"syscall"
	"time"

	"github.com/syncthing/syncthing/lib/build"
	"github.com/syncthing/syncthing/lib/config"
	"github.com/syncthing/syncthing/lib/dialer"
	"github.com/syncthing/syncthing/lib/events"
	"github.com/syncthing/syncthing/lib/fs"
	"github.com/syncthing/syncthing/lib/locations"
	"github.com/syncthing/syncthing/lib/logger"
	"github.com/syncthing/syncthing/lib/osutil"
	"github.com/syncthing/syncthing/lib/protocol"
	"github.com/syncthing/syncthing/lib/syncthing"
	"github.com/syncthing/syncthing/lib/tlsutil"
	"github.com/syncthing/syncthing/lib/upgrade"

	"github.com/pkg/errors"
)

const (
	tlsDefaultCommonName   = "syncthing"
	deviceCertLifetimeDays = 20 * 365
)

const (
	usage      = "syncthing [options]"
	extraUsage = `
The -logflags value is a sum of the following:

   1  Date
   2  Time
   4  Microsecond time
   8  Long filename
  16  Short filename

I.e. to prefix each log line with date and time, set -logflags=3 (1 + 2 from
above). The value 0 is used to disable all of the above. The default is to
show time only (2).


Development Settings
--------------------

The following environment variables modify Syncthing's behavior in ways that
are mostly useful for developers. Use with care.