Page MenuHomeFreeBSD

[libsa][NFC] ANSI libsa functions
Needs ReviewPublic

Authored by gfunni234_gmail.com on Aug 24 2021, 5:39 PM.

Details

Reviewers
imp
jrtc27
dim
Summary

A lot of the libsa functions were written back when ANSI C wasn’t a thing. So, I replaced their old, nonstandard parts with their modern-day equivalents.

No functional changes intended.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gfunni234_gmail.com retitled this revision from [libsa] ANSI libsa functions to [libsa][NFC] ANSI libsa functions.Aug 24 2021, 5:50 PM
gfunni234_gmail.com edited the summary of this revision. (Show Details)

ANSIfying definitions has some value, but everything else seems like a complete waste of time to me...

stand/libsa/ext2fs.c
801

This doesn't look very NFC to me

903

This is a bit different in nature, whole block ends up being indented... not saying it's better as it is but this seems like busywork for no appreciable gain, the current code is pretty clear

stand/libsa/in_cksum.c
91

& is commutative, this has zero semantic effect

stand/libsa/printf.c
75 ↗(On Diff #94114)

??? for all of these

stand/libsa/smbios.c
215 ↗(On Diff #94114)

Yes these variables are redundant. No this is not worthwhile to change.

516 ↗(On Diff #94114)

Equivalent, even if the old code goes against our current style(9)

stand/libsa/strcasecmp.c
54

Given that tolower is an inline function I doubt this has any effect on the final code generated, these are all simple transformations for half-decent compilers to make.

stand/libsa/strdup.c
48

You don't need this cast

48–50

style(9) has all variables declared at the start of the function, with the sole exception being that for loops may declare their counter variable

stand/libsa/ext2fs.c
801

NULL is 0. If fp is null, f->f_fsdata would be 0 to begin with, so this is an extra write.

903

I mean it was written back when do while wasn't standard c, which is why I think it would be an appropriate change

gfunni234_gmail.com marked an inline comment as done.
gfunni234_gmail.com marked 2 inline comments as done.
stand/libsa/in_cksum.c
91

just easier to read since & 0xffff is the same as % 0x10000

stand/libsa/ext2fs.c
801

And thus you see one of the problems with posting reviews that are a mess of unrelated, unnecessary changes like this: reviewers find it hard to review them and are more likely to make mistakes.

Can you split this review into at least two. So convert only the K&R style function decls to ANSI and the rest? The rest likely need to be split as well further, but that would at least give us an easy to review chunk. The goto again -> do while likely are a second chunk.
For changes like this where there's relatively little gain from the actual change, having small, committable chunks will speed the process. Extracting the prototypes changes, for example, is more hassle than it's worth for me, but cherry-picking a change isn't so bad.
It's also to MFC this way, and easier to back out should one subset of the changes prove to be bad.