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

apply the megacheck code vetting tool for idiomatic go #3949

Merged
merged 7 commits into from
Jun 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .codeclimate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ exclude_paths:
engines:
fixme:
enabled: true
config:
strings:
- FIXME
- HACK
- XXX
- BUG
golint:
enabled: true
govet:
Expand Down
2 changes: 1 addition & 1 deletion Rules.mk
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ include $(dir)/Rules.mk
# core targets #
# -------------------- #


build: $(TGT_BIN)
.PHONY: build

Expand Down Expand Up @@ -143,6 +142,7 @@ help:
@echo ' test_go_short'
@echo ' test_go_expensive'
@echo ' test_go_race'
@echo ' test_go_megacheck' - Run the `megacheck` vetting tool
@echo ' test_sharness_short'
@echo ' test_sharness_expensive'
@echo ' test_sharness_race'
Expand Down
3 changes: 1 addition & 2 deletions blocks/blocks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,8 @@ func TestManualHash(t *testing.T) {

u.Debug = true

block, err = NewBlockWithCid(data, c)
_, err = NewBlockWithCid(data, c)
if err != ErrWrongHash {
t.Fatal(err)
}

}
2 changes: 1 addition & 1 deletion blocks/blockstore/arc_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func testArcCached(ctx context.Context, bs Blockstore) (*arccache, error) {
func createStores(t *testing.T) (*arccache, Blockstore, *callbackDatastore) {
cd := &callbackDatastore{f: func() {}, ds: ds.NewMapDatastore()}
bs := NewBlockstore(syncds.MutexWrap(cd))
arc, err := testArcCached(nil, bs)
arc, err := testArcCached(context.TODO(), bs)
if err != nil {
t.Fatal(err)
}
Expand Down
9 changes: 2 additions & 7 deletions blocks/blockstore/blockstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,6 @@ func NewBlockstore(d ds.Batching) Blockstore {
type blockstore struct {
datastore ds.Batching

lk sync.RWMutex
gcreq int32
gcreqlk sync.Mutex

rehash bool
}

Expand Down Expand Up @@ -246,9 +242,8 @@ func NewGCLocker() GCLocker {
}

type gclocker struct {
lk sync.RWMutex
gcreq int32
gcreqlk sync.Mutex
lk sync.RWMutex
gcreq int32
}

// Unlocker represents an object which can Unlock
Expand Down
2 changes: 1 addition & 1 deletion blocks/blockstore/bloom_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func (b *bloomcache) hasCached(k *cid.Cid) (has bool, ok bool) {
}
if b.BloomActive() {
blr := b.bloom.HasTS(k.Bytes())
if blr == false { // not contained in bloom is only conclusive answer bloom gives
if !blr { // not contained in bloom is only conclusive answer bloom gives
b.hits.Inc()
return false, true
}
Expand Down
7 changes: 5 additions & 2 deletions blocks/blockstore/bloom_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ func TestPutManyAddsToBloom(t *testing.T) {
defer cancel()

cachedbs, err := testBloomCached(ctx, bs)
if err != nil {
t.Fatal(err)
}

select {
case <-cachedbs.rebuildChan:
Expand All @@ -49,15 +52,15 @@ func TestPutManyAddsToBloom(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if has == false {
if !has {
t.Fatal("added block is reported missing")
}

has, err = cachedbs.Has(block2.Cid())
if err != nil {
t.Fatal(err)
}
if has == true {
if has {
t.Fatal("not added block is reported to be in blockstore")
}
}
Expand Down
13 changes: 8 additions & 5 deletions blocks/blockstore/caching_test.go
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
package blockstore

import "testing"
import (
"context"
"testing"
)

func TestCachingOptsLessThanZero(t *testing.T) {
opts := DefaultCacheOpts()
opts.HasARCCacheSize = -1

if _, err := CachedBlockstore(nil, nil, opts); err == nil {
if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
t.Error("wrong ARC setting was not detected")
}

opts = DefaultCacheOpts()
opts.HasBloomFilterSize = -1

if _, err := CachedBlockstore(nil, nil, opts); err == nil {
if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
t.Error("negative bloom size was not detected")
}

opts = DefaultCacheOpts()
opts.HasBloomFilterHashes = -1

if _, err := CachedBlockstore(nil, nil, opts); err == nil {
if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
t.Error("negative hashes setting was not detected")
}
}
Expand All @@ -29,7 +32,7 @@ func TestBloomHashesAtZero(t *testing.T) {
opts := DefaultCacheOpts()
opts.HasBloomFilterHashes = 0

if _, err := CachedBlockstore(nil, nil, opts); err == nil {
if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
t.Error("zero hashes setting with positive size was not detected")
}
}
3 changes: 0 additions & 3 deletions blocks/set/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,11 @@
package set

import (
logging "gx/ipfs/QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52/go-log"
cid "gx/ipfs/QmYhQaCYEcaPPjxJX7YcPcVKkQfRy6sJ7B3XmGFk82XYdQ/go-cid"

"github.com/ipfs/go-ipfs/blocks/bloom"
)

var log = logging.Logger("blockset")

// BlockSet represents a mutable set of blocks CIDs.
type BlockSet interface {
AddBlock(*cid.Cid)
Expand Down
8 changes: 4 additions & 4 deletions blocks/set/set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ func exampleKeys() []*cid.Cid {
func checkSet(set BlockSet, keySlice []*cid.Cid, t *testing.T) {
for i, key := range keySlice {
if i&tReAdd == 0 {
if set.HasKey(key) == false {
if !set.HasKey(key) {
t.Error("key should be in the set")
}
} else if i&tRemove == 0 {
if set.HasKey(key) == true {
if set.HasKey(key) {
t.Error("key shouldn't be in the set")
}
} else if i&tAdd == 0 {
if set.HasKey(key) == false {
if !set.HasKey(key) {
t.Error("key should be in the set")
}
}
Expand Down Expand Up @@ -70,7 +70,7 @@ func TestSetWorks(t *testing.T) {
bloom := set.GetBloomFilter()

for _, key := range addedKeys {
if bloom.Find(key.Bytes()) == false {
if !bloom.Find(key.Bytes()) {
t.Error("bloom doesn't contain expected key")
}
}
Expand Down
2 changes: 1 addition & 1 deletion blockservice/blockservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (s *blockService) GetBlock(ctx context.Context, c *cid.Cid) (blocks.Block,
// the returned channel.
// NB: No guarantees are made about order.
func (s *blockService) GetBlocks(ctx context.Context, ks []*cid.Cid) <-chan blocks.Block {
out := make(chan blocks.Block, 0)
out := make(chan blocks.Block)
go func() {
defer close(out)
var misses []*cid.Cid
Expand Down
9 changes: 3 additions & 6 deletions cmd/ipfs/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,9 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
ctx := req.InvocContext()

go func() {
select {
case <-req.Context().Done():
fmt.Println("Received interrupt signal, shutting down...")
fmt.Println("(Hit ctrl-c again to force-shutdown the daemon.)")
}
<-req.Context().Done()
fmt.Println("Received interrupt signal, shutting down...")
fmt.Println("(Hit ctrl-c again to force-shutdown the daemon.)")
}()

// check transport encryption flag.
Expand Down Expand Up @@ -418,7 +416,6 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
res.SetError(err, cmds.ErrNormal)
}
}
return
}

// serveHTTPApi collects options, creates listener, prints status message and starts serving requests
Expand Down
15 changes: 4 additions & 11 deletions cmd/ipfs/ipfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ func init() {
}
}

// isLocal returns true if the command should only be run locally (not sent to daemon), otherwise false
func isLocal(cmd *cmds.Command) bool {
_, found := localMap[cmd]
return found
}

// NB: when necessary, properties are described using negatives in order to
// provide desirable defaults
type cmdDetails struct {
Expand Down Expand Up @@ -85,11 +79,10 @@ func (d *cmdDetails) Loggable() map[string]interface{} {
}
}

func (d *cmdDetails) usesConfigAsInput() bool { return !d.doesNotUseConfigAsInput }
func (d *cmdDetails) doesNotPreemptAutoUpdate() bool { return !d.preemptsAutoUpdate }
func (d *cmdDetails) canRunOnClient() bool { return !d.cannotRunOnClient }
func (d *cmdDetails) canRunOnDaemon() bool { return !d.cannotRunOnDaemon }
func (d *cmdDetails) usesRepo() bool { return !d.doesNotUseRepo }
func (d *cmdDetails) usesConfigAsInput() bool { return !d.doesNotUseConfigAsInput }
func (d *cmdDetails) canRunOnClient() bool { return !d.cannotRunOnClient }
func (d *cmdDetails) canRunOnDaemon() bool { return !d.cannotRunOnDaemon }
func (d *cmdDetails) usesRepo() bool { return !d.doesNotUseRepo }

// "What is this madness!?" you ask. Our commands have the unfortunate problem of
// not being able to run on all the same contexts. This map describes these
Expand Down
11 changes: 3 additions & 8 deletions cmd/ipfs/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,12 @@ import (
// log is the command logger
var log = logging.Logger("cmd/ipfs")

var (
errUnexpectedApiOutput = errors.New("api returned unexpected output")
errApiVersionMismatch = errors.New("api version mismatch")
errRequestCanceled = errors.New("request canceled")
)
var errRequestCanceled = errors.New("request canceled")

const (
EnvEnableProfiling = "IPFS_PROF"
cpuProfile = "ipfs.cpuprof"
heapProfile = "ipfs.memprof"
errorFormat = "ERROR: %v\n\n"
)

type cmdInvocation struct {
Expand Down Expand Up @@ -492,7 +487,7 @@ func startProfiling() (func(), error) {
}
pprof.StartCPUProfile(ofi)
go func() {
for _ = range time.NewTicker(time.Second * 30).C {
for range time.NewTicker(time.Second * 30).C {
err := writeHeapProfileToFile()
if err != nil {
log.Error(err)
Expand Down Expand Up @@ -546,7 +541,7 @@ func (ih *IntrHandler) Handle(handler func(count int, ih *IntrHandler), sigs ...
go func() {
defer ih.wg.Done()
count := 0
for _ = range ih.sig {
for range ih.sig {
count++
handler(count, ih)
}
Expand Down
8 changes: 3 additions & 5 deletions cmd/ipfswatch/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"os/signal"
"path/filepath"
"syscall"

commands "github.com/ipfs/go-ipfs/commands"
core "github.com/ipfs/go-ipfs/core"
Expand Down Expand Up @@ -99,7 +100,7 @@ func run(ipfsPath, watchPath string) error {
}

interrupts := make(chan os.Signal)
signal.Notify(interrupts, os.Interrupt, os.Kill)
signal.Notify(interrupts, os.Interrupt, syscall.SIGTERM)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is os.Kill deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the error message is: os.Kill cannot be trapped (did you mean syscall.SIGTERM?) (SA1016) where SA1016 -> Trapping a signal that cannot be trapped


for {
select {
Expand Down Expand Up @@ -167,10 +168,7 @@ func addTree(w *fsnotify.Watcher, root string) error {
}
return nil
})
if err != nil {
return err
}
return nil
return err
}

func IsDirectory(path string) (bool, error) {
Expand Down
14 changes: 1 addition & 13 deletions commands/cli/helptext.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ const (
variadicArg = "%v..."
shortFlag = "-%v"
longFlag = "--%v"
optionType = "(%v)"

whitespace = "\r\n\t "

indentStr = " "
)
Expand Down Expand Up @@ -295,9 +292,7 @@ func optionText(cmd ...*cmds.Command) []string {
// get a slice of the options we want to list out
options := make([]cmds.Option, 0)
for _, c := range cmd {
for _, opt := range c.Options {
options = append(options, opt)
}
options = append(options, c.Options...)
}

// add option names to output (with each name aligned)
Expand Down Expand Up @@ -427,13 +422,6 @@ func align(lines []string) []string {
return lines
}

func indent(lines []string, prefix string) []string {
for i, line := range lines {
lines[i] = prefix + indentString(line, prefix)
}
return lines
}

func indentString(line string, prefix string) string {
return prefix + strings.Replace(line, "\n", "\n"+prefix, -1)
}
Expand Down
5 changes: 1 addition & 4 deletions commands/cli/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,8 @@ func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *c
}

err = cmd.CheckArguments(req)
if err != nil {
return req, cmd, path, err
}

return req, cmd, path, nil
return req, cmd, path, err
}

func ParseArgs(req cmds.Request, inputs []string, stdin *os.File, argDefs []cmds.Argument, root *cmds.Command) ([]string, []files.File, error) {
Expand Down
2 changes: 1 addition & 1 deletion commands/cli/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func TestArgumentParsing(t *testing.T) {

test := func(cmd words, f *os.File, res words) {
if f != nil {
if _, err := f.Seek(0, os.SEEK_SET); err != nil {
if _, err := f.Seek(0, io.SeekStart); err != nil {
t.Fatal(err)
}
}
Expand Down
1 change: 0 additions & 1 deletion commands/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package commands
import "testing"

func noop(req Request, res Response) {
return
}

func TestOptionValidation(t *testing.T) {
Expand Down
1 change: 0 additions & 1 deletion commands/files/multipartfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

const (
multipartFormdataType = "multipart/form-data"
multipartMixedType = "multipart/mixed"

applicationDirectory = "application/x-directory"
applicationSymlink = "application/symlink"
Expand Down
Loading