Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 17301
Event Timeline
I think this is good. I'm a little hesitant about the suffix part of it, but know of no good way to flag it's acceptance (I know of several ways, they are all complex and error-prone though).
so, I'd feel better with someone else also blessed the change.
kern/kern_environment.c | ||
---|---|---|
548 ↗ | (On Diff #43750) | I am confused by this loop control. You start with a random garbage on stack for the initial buf[] content. |
kern/kern_environment.c | ||
---|---|---|
548 ↗ | (On Diff #43750) | ptr = buf, means address of buf. buf is filled with a valid string after the getenv_string() call. |
kern/kern_environment.c | ||
---|---|---|
545 ↗ | (On Diff #43750) | The comment is useless. |
548 ↗ | (On Diff #43750) | Ah, I missed it. |
552 ↗ | (On Diff #43750) | Style: declare vars at the top of the function. |
554 ↗ | (On Diff #43750) | Well, almost all of the comments trivially rephrase the code. |
587 ↗ | (On Diff #43750) | This accepts arbitrary garbage after the numeric part. |
614 ↗ | (On Diff #43750) | Now I am confused by this comparision. Valid range for unsigned char on all supported arches is 0..255. |
619 ↗ | (On Diff #43750) | Same note for uint16_t. |
624 ↗ | (On Diff #43750) | And there. |
kern/kern_environment.c | ||
---|---|---|
614 ↗ | (On Diff #43750) | I'll fix. |
sys/kern/kern_environment.c | ||
---|---|---|
539 | I think it would be simpler to just add a 'bool signed' parameter instead of overloading type_size. type_size can then always be 'sizeof(foo)' which would be easier to read at the call-site. If you'd rather not use a bool, then define a simple enum: enum sign { SIGNED, UNSIGNED, }; And use that instead so you have something like this for CTLTYPE_UINT: error = getenv_array(path + rem, data, sizeof(data), &size, sizeof(int), UNSIGNED); | |
sys/kern/kern_sysctl.c | ||
195–196 | Using a uint64_t array initially threw me off. I wonder if it wouldn't be more accurate to declare this as 'char data[1024] __Alignas(uint64_t)' or the like? |
sys/sys/systm.h | ||
---|---|---|
349 | I would perhaps move these constants up out of the list of function prototypes, however the prevailing style in this file seems to be to include them without a comment below a function prototype followed by a blank line before the next function prototype (see HD_* after hexdump(), HASH_* after hashinit_flags()). |