Page MenuHomeFreeBSD

style: Tighten up one use of 'may'
ClosedPublic

Authored by imp on Tue, Jul 26, 8:23 PM.

Details

Summary

Declarations of variables must be placed before the executable lines of
a block, by convention. Use 'must' instead of 'may' here and clarify wording.

Diff Detail

Repository
rG 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

imp requested review of this revision.Tue, Jul 26, 8:23 PM
This revision is now accepted and ready to land.Tue, Jul 26, 8:23 PM
cperciva added inline comments.
share/man/man9/style.9
703–704

I think the "may" refers to the "at the start of any block". i.e. what is intended is "Declarations must be placed before executable lines, and may occur at the start of any block."

Improve with Collin's suggestion

This revision now requires review to proceed.Tue, Jul 26, 8:28 PM
pstef added inline comments.
share/man/man9/style.9
703–704

I think this is a good suggestion.
I would go further and replace "executable lines" with "statements" to follow the clang message.

This revision is now accepted and ready to land.Tue, Jul 26, 8:39 PM
emaste added inline comments.
share/man/man9/style.9
703–704

I like pstef's suggestion.

share/man/man9/style.9
538

We have only 307 NOTREACHEDs in the tree, in 121 files. annotations (like __dead2 or __assert_unreachable as some folks mentioned on IRC) are much better

__assert_unreachable is the more modern spelling of /* NOTREACHED */ so
recommend that. Use statement (the language the C standard uses) rather than the
more vague executable lines.

This revision now requires review to proceed.Tue, Jul 26, 9:00 PM

mjg points out this isn't in userland.

share/man/man9/style.9
538
Code which is unreachable for non-obvious reasons should be marked accordingly.
In the kernel,
.Fn __assert_unreachable
should be used in preference to the older /*
.Li NOTREACHED
*/.
That annotation is not available in userland,
so use /*
.Li NOTREACHED
*/ there instead.
jhb added inline comments.
share/man/man9/style.9
538

I'd rather just expose __assert_unreachable in userland.

I don't read the previous version as encouraging the use of annotations, just permitting them. The new version reads to me as if it more strongly mandates it. That is, I feel like the old use of may here was actually correct.

share/man/man9/style.9
538

I think I'll drop these chunks of the change as 'premature' then. We'll revisit once we have different interfaces and want a stronger encouragement.

imp retitled this revision from style: Tighten up some uses of 'may' to style: Tighten up one use of 'may'.Tue, Jul 26, 9:47 PM
imp edited the summary of this revision. (Show Details)

drop notreached... it's not quite ready

imp marked 6 inline comments as done.Tue, Jul 26, 9:48 PM

Worth adding a short description of must/should/may/should not/must not a la RFC2119?

share/man/man9/style.9
2
28

Bump.

41
hselasky added a subscriber: hselasky.

No objections.

This revision is now accepted and ready to land.Thu, Jul 28, 1:57 PM
This revision was automatically updated to reflect the committed changes.

So that's a 'no' on expanding the scope of this review to have full RFC-language. I may do that in the future, but I don't
have time for it now.