Page MenuHomeFreeBSD
Differential D32052

tests: Test endian.h, byteswap.h, sys/endian.h and both endian.h and byteswap.h together
ClosedPublic
Actions

Authored by imp on Sep 21 2021, 5:08 PM.
Tags
None
Referenced Files
F168987151: D32052.id95471.diff
Mon, Aug 31, 1:43 AM
Unknown Object (File)
Sun, Aug 30, 8:37 AM
Unknown Object (File)
Sat, Aug 29, 11:56 PM
Unknown Object (File)
Sat, Aug 29, 7:30 PM
Unknown Object (File)
Sat, Aug 29, 1:10 PM
Unknown Object (File)
Sat, Aug 29, 12:31 PM
Unknown Object (File)
Sat, Aug 29, 12:31 PM
Unknown Object (File)
Sat, Aug 29, 1:44 AM
View All Files
Subscribers
arichardson
lwhsu

Details

Reviewers
emaste
jhb
markj
kevans
Commits
rGac77b2621508: tests: Test endian.h, byteswap.h, sys/endian.h and both endian.h and byteswap.h…
Summary

What's required and not required to be defined is complicated. Write
tests to enshrine it:
endian.h and sys/endian.h:

		[bl]e{16,32,64}toh
		hto[bl]e{16,32,64}

byteswap.h:

		{__,}bswap_{16,32,64}

sys/endian.h:

		{__,}bswap{16,32,64}
		_BYTE_ORDER
		_BIG_ENDIAN
		_LITTLE_ENDIAN
		_PDP_ENDIAN

endian.h:

		__BYTE_ORDER
		__BIG_ENDIAN
		__LITTLE_ENDIAN
		__PDP_ENDIAN
		__FLOAT_WORD_ORDER

NOT TESTED: deprecated symbols, internal to glibc symbols

Sponsored by: Netflix

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp requested review of this revision.Sep 21 2021, 5:08 PM
imp created this revision.
Harbormaster completed remote builds in B41626: Diff 95460.Sep 21 2021, 5:08 PM
imp added reviewers: emaste, jhb, markj.Sep 21 2021, 5:20 PM
imp added a comment.Sep 21 2021, 5:34 PM

If anybody knows an easier, less repetitive way to do this, I'm all ears.

kevans added a reviewer: kevans.Sep 21 2021, 5:49 PM
imp updated this revision to Diff 95471.Sep 21 2021, 5:59 PM

Kyle made a suggestion to make it a lot simpler, use it.

Harbormaster completed remote builds in B41628: Diff 95471.Sep 21 2021, 5:59 PM
imp updated this revision to Diff 95480.Sep 21 2021, 7:29 PM

include both orders for endian.h and sys/endian.h

Harbormaster completed remote builds in B41636: Diff 95480.Sep 21 2021, 7:29 PM
imp added parent revisions: D32051: byteswap.h: Add a glibc/linux compatible byteswap.h, D31962: linux: For better compatibility, provide compatible endian.h.Sep 21 2021, 7:29 PM
arichardson added a subscriber: arichardson.Sep 22 2021, 7:53 AM
arichardson added inline comments.
include/tests/sys_endian_test.c
21 ↗(On Diff #95480)

Could use #error for these checks since it's done at compile time already?

imp added inline comments.Sep 22 2021, 1:10 PM
include/tests/sys_endian_test.c
21 ↗(On Diff #95480)

I don't think atf can give a proper count if you do that... I don't see other things in the tree do that...

arichardson added inline comments.Sep 22 2021, 1:21 PM
include/tests/sys_endian_test.c
21 ↗(On Diff #95480)

Yes, atf won't report it, but it will be reported (earlier) as a build failure.

lwhsu added a subscriber: lwhsu.Sep 22 2021, 2:48 PM

This seems creating a new directory /usr/tests/include, do we also need to modify etc/mtree/BSD.tests.dist ?

imp added a comment.Sep 22 2021, 5:01 PM
In D32052#723399, @lwhsu wrote:

This seems creating a new directory /usr/tests/include, do we also need to modify etc/mtree/BSD.tests.dist ?

Yes. I hadn't through of that, but I can't imagine not needing it.

include/tests/sys_endian_test.c
21 ↗(On Diff #95480)

I'll let others lead the way. There's just a couple of math tests that use it for a floating point size error...

imp updated this revision to Diff 95588.Sep 23 2021, 10:27 PM

rebase / tweak

Harbormaster completed remote builds in B41682: Diff 95588.Sep 23 2021, 10:27 PM
markj added inline comments.Sep 24 2021, 1:22 PM
include/Makefile
463 ↗(On Diff #95588)

I suspect tests/include would be a more natural place for these tests. We generally try to pair tests with the corresponding executable or library sources, but I'm not sure that makes much sense here - it doesn't seem easy to find include/tests, and I'd naively expect that directory to just contain more header files, like other directories under include/.

include/tests/byteswap_test.c
23 ↗(On Diff #95588)

style(9) says to avoid C++-style comments.

include/tests/sys_endian_endian_test.c
8 ↗(On Diff #95588)
8 ↗(On Diff #95588)

The comment should be formatted as a single block.

imp added inline comments.Sep 24 2021, 9:11 PM
include/Makefile
463 ↗(On Diff #95588)

Yea, I was a bit torn on this...
One could make an argument too for some place under libc, but I didn't find a place to put them.
I'll rename if it's easy, and leave them here if it isn't.

include/tests/byteswap_test.c
23 ↗(On Diff #95588)

I think it used to say that, but doesn't anymore.
It doesn't use them at all, and gives /* */ as the way to do one line comments
I couldn't find the change that removed that advice, though.

I'll fix this if I wind up moving things to tests/include.

include/tests/sys_endian_endian_test.c
8 ↗(On Diff #95588)

OK.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 15 2024, 11:15 PM
Closed by commit rGac77b2621508: tests: Test endian.h, byteswap.h, sys/endian.h and both endian.h and byteswap.h… (authored by imp). · Explain Why
This revision was automatically updated to reflect the committed changes.
imp added a commit: rGac77b2621508: tests: Test endian.h, byteswap.h, sys/endian.h and both endian.h and byteswap.h….

Revision Contents
Changeset List

Diff 144961

View Options

tests/include/Makefile

Loading...
View Options

tests/include/byteswap_endian_test.c

Loading...
View Options

tests/include/byteswap_test.c

Loading...
View Options

tests/include/endian_sys_endian_test.c

Loading...
View Options

tests/include/endian_test.c

Loading...
View Options

tests/include/sys_endian_endian_test.c

Loading...
View Options

tests/include/sys_endian_test.c

Loading...