lint: change vet -shadow to subtest calling -vettool=shadow #36486
Conversation
|
This change is |
|
looks like opt and exec both have some shadowing in generated code that we might want to either ignore or fix. staticcheck also failed for me with some unexpected errors like |
There was a problem hiding this comment.
LGTM. I assume this version works for both go 1.11 and 1.12?
The "identical expressions" thing is dominikh/go-tools#430
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
|
I checked that I need to check with opt and exec about those shadowing errors, then I think this is okay for master / go 1.11 as is, and I'll file a separate issue for the assignment thing. |
|
I fixed the filtering of shadowing errors from generated code (it has started including line offset in with line number which broke the regex). |
There was a problem hiding this comment.
Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 1301 at r2 (raw file):
stream.GrepNot(`\.[eo]g\.go:[0-9:]+: declaration of ".*" shadows`), stream.GrepNot(`^#`), // comment line // Upstream compiler error. See: https://github.com/golang/go/issues/23701
Can we get rid of this now that golang/go#23701 is closed?
pkg/testutils/lint/lint_test.go, line 1346 at r2 (raw file):
}, ",") // unfortunately if an analyzer is passed via -vettool like shadow, it seems
Capital "unfortunately"
-shadow was removed in go 1.12. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 1301 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Can we get rid of this now that golang/go#23701 is closed?
Done.
pkg/testutils/lint/lint_test.go, line 1346 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Capital "unfortunately"
Done.
|
bors r+ |
36445: storage: avoid double-ingest of same SST on re-apply r=dt a=dt If an AddSSTable is re-applied when using the no-copy, hard-link only ingest, the second ingest passes a link to the same underlying file (by inode) to rocks. Due to facebook/rocksdb#5133, this then causes the ingestion to fail. This avoids that by checking if the file we're about to ingest already has moore than one link. If so, we do not allow ingesting the raft file directly and instead make a copy. Release note: none. 36486: lint: change vet -shadow to subtest calling -vettool=shadow r=dt a=dt -shadow was removed in go 1.12. Release note: None 36502: backfill: lower default backfiller batch size to 5000 r=lucy-zhang a=lucy-zhang Lower `schemachanger.bulk_index_backfill.batch_size` from 5000000 to 5000 to reduce memory allocation when buffering KVs during index backfills, now that KVs are buffered and sorted in BulkAdder instead of being sorted in every batch. Release note: None Co-authored-by: David Taylor <[email protected]> Co-authored-by: Lucy Zhang <[email protected]>
Build succeeded |
|
@bdarnell should we backport this and #36527 to make linting on release branch also happy on 1.12? On the one hand, they're test-only changes so I'm not too worried about them. On the other hand, the CI will stay on go 1.11 thanks to the builder so it'd mostly be for local dev, once people have upgraded to 1.12, but personally, I don't usually run |
|
I think the frozen builder is good enough for when we're working on older branches, so I wouldn't backport this (but I also wouldn't object to backporting it). |
-shadow was removed in go 1.12.
Release note: None