Fix CTASSERT issue in a more clean way
ClosedPublic
Actions

Authored by ngie on Jul 6 2016, 6:27 AM.

Details

Reviewers

grehan
neel
ed

Summary

Replace all CTASSERT macro instances with static_assert's
Localize all static_assert's need their relevant structures being tested

MFC after: 1 week
X-MFC with: r302364
Reviewed by: grehan (maintainer)
Submitted by: ed
Sponsored by: EMC / Isilon Storage Division

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

ngie updated this revision to Diff 18176.Jul 6 2016, 6:27 AM

ngie retitled this revision from to Fix CTASSERT issue in a more clean way.

ngie updated this object.

ngie edited the test plan for this revision. (Show Details)

ngie added reviewers: neel, grehan.

ngie set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJul 6 2016, 6:27 AM

ngie added subscribers: ed, markj, cem.Jul 6 2016, 6:28 AM

grehan accepted this revision.Jul 6 2016, 6:30 AM

grehan edited edge metadata.

This revision is now accepted and ready to land.Jul 6 2016, 6:30 AM

ed requested changes to this revision.Jul 6 2016, 6:38 AM

ed added a reviewer: ed.

ed added inline comments.

pci_emul.c
758	We originally had to place these `CTASSERT()`s in source files as opposed to header files, as our version of the macro was implemented with `__LINE__` instead of `__COUNTER__`. Could you please move these compile-time assertions next to the actual structure declarations (i.e. into `pci_emul.h`)? That way they're less likely to be removed accidentally due to refactoring, tested for every use, etc. Also: why the indirection? `_Static_assert()` is part of the C standard and supported by FreeBSD 9 and higher, even if you're using an ancient compiler (GCC 4.2). Strongly consider removing `CTASSERT()` altogether. There is no use for it. Looking at `pci_emul.h`, it already includes `<assert.h>`, so you can even just use `static_assert()`.

This revision now requires changes to proceed.Jul 6 2016, 6:38 AM

ngie marked an inline comment as done.Jul 6 2016, 6:47 AM

ngie added inline comments.

pci_emul.c
758	Done.

Replace all CTASSERT macro instances with static_assert's
Localize all static_assert's need their relevant structures being tested

ngie marked an inline comment as done.Jul 6 2016, 6:58 AM

Thanks, much nicer.

Looks good! For bonus points, you could also customise the error strings to something like "Required by PCI standard abc paragraph xyz", but I don't care about that too strongly. Future versions of _Static_assert() may even make the error message optional anyway:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3928.pdf

This revision is now accepted and ready to land.Jul 6 2016, 7:01 AM

In D7130#148571, @ed wrote:

Looks good! For bonus points, you could also customise the error strings to something like "Required by PCI standard abc paragraph xyz", but I don't care about that too strongly. Future versions of _Static_assert() may even make the error message optional anyway:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3928.pdf

"" seems to be a secondary option that's used elsewhere in the tree. I kind of lack the patience to look up the PCI standard spec that states that the structure must be packed in a particular way :P, and eventually this will become optional.. it's just not available in this particular standard version (and I don't want to go chasing down more issues right now.. this works "well enough" for ^/stable/11).

rS302373