Skip to content

Gazelle: remove support for graphical multidiff#357

Merged
jayconrod merged 1 commit intobazel-contrib:masterfrom
jayconrod:fix-gazelle-multidiff
Apr 4, 2017
Merged

Gazelle: remove support for graphical multidiff#357
jayconrod merged 1 commit intobazel-contrib:masterfrom
jayconrod:fix-gazelle-multidiff

Conversation

@jayconrod
Copy link
Copy Markdown
Collaborator

buildifier/differ supports both graphical and non-graphical
diff. The graphical diff doesn't work though: it generates a
command line for tkdiff, but tkdiff doesn't actually accept commands
in that format.

With this change, we just print diffs in the simple unified format by
executing diff -u.

Fixes #342

buildifier/differ supports both graphical and non-graphical
diff. The graphical diff doesn't work though: it generates a
command line for tkdiff, but tkdiff doesn't actually accept commands
in that format.

With this change, we just print diffs in the simple unified format by
executing `diff -u`.

Fixes bazel-contrib#342
@pmbethe09
Copy link
Copy Markdown
Contributor

LGTM

@jayconrod jayconrod merged commit 4c55065 into bazel-contrib:master Apr 4, 2017
@jayconrod jayconrod deleted the fix-gazelle-multidiff branch April 4, 2017 19:14
TvdW pushed a commit to TvdW/rules_go that referenced this pull request Dec 23, 2025
…el-contrib#358)

This PR adds support for built-in conversion functions, such as
`string(x)`, used as index expr. Currently, without this support,
NilAway reports a false positive for the example below.
```
func test(v uint8) {
		m := make(map[string]*int)
		if m[string(v)] != nil {
			_ = *m[string(v)]  // false positive was reported here
		}
}
```

Go compiler doesn't treat `string(x)` as a call to a built-in function
named “string”, but instead it's considered to be a conversion using the
predeclared type string. Therefore, our existing logic of
`*types.BuiltIn` did not cover this case. We had to add special support
for this case by checking for
`pass.TypesInfo.ObjectOf(ident).Type().(*types.Basic)`.

[Closes bazel-contrib#357 ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants