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.
Tags
None
Referenced Files
Unknown Object (File)
Dec 22 2023, 10:39 PM
Unknown Object (File)
Nov 20 2023, 8:42 AM
Unknown Object (File)
Nov 18 2023, 7:16 AM
Unknown Object (File)
Nov 18 2023, 2:23 AM
Unknown Object (File)
Nov 18 2023, 1:50 AM
Unknown Object (File)
Nov 18 2023, 1:16 AM
Unknown Object (File)
Oct 24 2023, 9:32 AM
Unknown Object (File)
Oct 21 2023, 5:29 PM
Subscribers
None

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 24680

Event Timeline

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

It's never needed it

but it could be spelled more simply as

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

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.

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.

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

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
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. :-)

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.