Fix some clang warnings.
ClosedPublic
Actions

Authored by araujo on Jun 3 2015, 7:59 AM.

Details

Reviewers

• rodrigc
delphij
mahrens

Commits

rS284292: At revision r247852 accidentally was removed from print the variable obj.

Summary

At revision r247852 @mm accidentally we forgot to print out the obj too.
Fix some void function that don't really need to return.
Also for some global variables, declare it as static.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

araujo updated this revision to Diff 5906.Jun 3 2015, 7:59 AM

araujo retitled this revision from to Fix some clang warnings..

araujo updated this object.

araujo edited the test plan for this revision. (Show Details)

araujo added reviewers: • rodrigc, delphij.

Herald added a subscriber: imp. · View Herald TranscriptJun 3 2015, 7:59 AM

Please note that the two comments inline, I think they are wrong and should be reverted. The rest part of the proposed patch looks good to me.

cddl/contrib/opensolaris/cmd/zdb/zdb.c
182 ↗	(On Diff #5906)	This is wrong. usage() is supposed to be an exit point.
201 ↗	(On Diff #5906)	This is also wrong, similar to usage() fatal is supposed to be a __dead exit.

This revision now requires changes to proceed.Jun 7 2015, 4:58 AM

As @delphij pointed out, I have reverted those changes related with the 'exit(1);' case.

Adding Matt to reviewer so he gets notification. I think we should upstream most of the changes to Illumos -- Marcelo, can you create an Illumos ticket for this? I can create a review request there too but since you wrote the patch it's better that you get involved too :)

cddl/contrib/opensolaris/cmd/zdb/zdb.c
96 ↗	(On Diff #6013)	These changes should be upstreamed to Illumos.
1492 ↗	(On Diff #6013)	This should be upstreamed to Illumos.
1494 ↗	(On Diff #6013)	I'm not sure about the style here -- I think if I was the author I would have a blank line on after line 1490, but it's not clear if it also matches Illumos code style.
1500 ↗	(On Diff #6013)	This should be upstreamed too.

The revision seems good to me.

This revision is now accepted and ready to land.Jun 10 2015, 6:53 PM

mahrens added inline comments.Jun 10 2015, 10:02 PM

cddl/contrib/opensolaris/cmd/zdb/zdb.c
95 ↗	(On Diff #6013)	I don't see any references to this in zdb_il.c; can we make it "static" too?
1494 ↗	(On Diff #6013)	doesn't matter much either way to me.

In D2722#53037, @delphij wrote:

Adding Matt to reviewer so he gets notification. I think we should upstream most of the changes to Illumos -- Marcelo, can you create an Illumos ticket for this? I can create a review request there too but since you wrote the patch it's better that you get involved too :)

@delphij, Yes I can open a ticket there without any problem, I will do it today.