Page MenuHomeFreeBSD

Fix some clang warnings.
ClosedPublic

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

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.
delphij requested changes to this revision.Jun 7 2015, 4:58 AM
delphij edited edge metadata.

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
araujo updated this revision to Diff 6013.Jun 7 2015, 12:20 PM
araujo edited edge metadata.

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.

delphij accepted this revision.Jun 10 2015, 6:53 PM
delphij edited edge metadata.

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.

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.

araujo updated this revision to Diff 6095.Jun 11 2015, 1:29 AM
araujo edited edge metadata.

@mahrens thanks for the review, yes, that supposed to be static too.
Please check this new version of the patch.

Best,

This revision now requires review to proceed.Jun 11 2015, 1:29 AM

The revision seems good to me.

@delphij I had already open a BUG at Illumos project, here is the link: https://www.illumos.org/issues/5999

Thanks,

mahrens accepted this revision.Jun 11 2015, 3:03 PM
mahrens edited edge metadata.
This revision is now accepted and ready to land.Jun 11 2015, 3:03 PM
delphij accepted this revision.Jun 11 2015, 11:25 PM
delphij edited edge metadata.