Page MenuHomeFreeBSD

isa/isa_common.c: remove needless if (!rl) check in isa_get_resource_list()
AcceptedPublic

Authored by danfe on Jun 3 2019, 5:35 PM.

Details

Reviewers
jhb
imp
Summary

PVS-Studio reports: /usr/src/sys/isa/isa_common.c:968:1: warning: V547 Expression '!rl' is always false.

I'm not sure if this diagnostic is 100% correct, but couldn't help to notice that the whole if condition is actually useless: it looks like simple return (rl) would do the right thing on its own. Consider attached patch.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24680

Event Timeline

danfe created this revision.Jun 3 2019, 5:35 PM
jhb accepted this revision.Jun 3 2019, 6:42 PM

The function was like this when it was first committed.

This revision is now accepted and ready to land.Jun 3 2019, 6:42 PM
imp accepted this revision.Jun 3 2019, 7:16 PM

It's never needed it

but it could be spelled more simply as

return (&DEVTOISA(child)->id_resources);

danfe added a comment.Jun 4 2019, 9:37 AM

Warner, do you want me to update the diff to use return (&DEVTOISA(child)->id_resources);? I'm a bit worried that it would cause more repochurn compared to the current diff which only removes the lines and thus does not affect svn blame on the the others.

imp added a comment.Jun 4 2019, 2:12 PM

Warner, do you want me to update the diff to use return (&DEVTOISA(child)->id_resources);? I'm a bit worried that it would cause more repochurn compared to the current diff which only removes the lines and thus does not affect svn blame on the the others.

I wouldn't worry about the repo-churn. We move things around all the time, and finding out the history of this won't be difficult for future archaeologists.

danfe updated this revision to Diff 58231.Jun 4 2019, 3:39 PM

Elide local variables and return resource list in place as suggested by @imp.

This revision now requires review to proceed.Jun 4 2019, 3:39 PM
jhb accepted this revision.Jun 4 2019, 11:19 PM

I almost suggested a variant of Warner's, but figured it wasn't worth the extra churn. :-P Fine that you've done it. (My version would have been to just remove rl and used 'return (&idev->id_resources)' only because the DEVTOISA as the first line is a kind of pattern/idiom in this file.)

This revision is now accepted and ready to land.Jun 4 2019, 11:19 PM
danfe added a comment.Jun 5 2019, 9:40 AM
In D20506#443247, @jhb wrote:

My version would have been to just remove rl and used return (&idev->id_resources) only because the DEVTOISA as the first line is a kind of pattern/idiom in this file.

I've also noticed the pattern/idiom and was not sure if it worth retaining it in this function. If you prefer, I can redo the diff.

In any case, as an non-src committer, I'd probably need an explicit approval from one of you since phab acceptance might not really imply it. Alternatively, feel free to commit (any version you like) yourselves. :-)

imp accepted this revision.Jun 5 2019, 3:36 PM

I'd just roll with this. IMHO, the simplicity trumps the other patterns in the file because using the pattern here gives no real benefit.