Page MenuHomeFreeBSD

Make "sys/queue.h" usable with C++
ClosedPublic

Authored by hselasky on Jun 6 2015, 7:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 3:45 AM
Unknown Object (File)
Wed, Mar 13, 8:07 AM
Unknown Object (File)
Fri, Mar 1, 9:07 PM
Unknown Object (File)
Fri, Mar 1, 9:06 PM
Unknown Object (File)
Fri, Mar 1, 9:05 PM
Unknown Object (File)
Fri, Mar 1, 9:05 PM
Unknown Object (File)
Fri, Mar 1, 9:05 PM
Unknown Object (File)
Fri, Mar 1, 9:05 PM

Details

Reviewers
wblock
vangyzen
Group Reviewers
manpages

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

hselasky retitled this revision from to Make "sys/queue.h" usable with C++.
hselasky updated this object.
hselasky edited the test plan for this revision. (Show Details)
hselasky set the repository for this revision to rS FreeBSD src repository - subversion.

Please upload diffs with full context, e.g. via git diff -U99999 or svn diff --diff-cmd=diff -x -U999999

https://wiki.freebsd.org/CodeReview

I'm hardly an expert on the queue macros - or C++ - but this change seems reasonable to me. I assume `make universe' works?

Yes, I'm going to push it through "make universe" before commit.

Since the root problem seems to be the assumption that "type" is a struct, why not simply omit the "struct" keyword under C++? For example:

#ifdef __cplusplus
#define __sys_queue_struct
#else
#define __sys_queue_struct struct
#endif

Of course, change most instances of "struct" to __sys_queue_struct, and #undef this at the end of queue.h. I'm running buildworld with this patch now.

As it turns out, my suggestion would need extra work. In some places, "type" is not declared enough to appease C++. For example, ktr_request in sys/proc.h, callout in sys/_callout.h, and aiocblist in sys/socketvar.h. These are trivially fixed with forward declarations, or by reordering the declarations. However, out-of-tree consumers might require changes. Thoughts?

Some better explanation is in order. I've used the sys/queue.h macros in the past in C++ with no issues. The extra stuff you've done seems unnecessary. What's the problem that having the _CLASS stuff solves, and why do you need the non-standard __typeof?

sys/sys/queue.h
163

Why is this needed? Class and struct are the same thing in C++, with the definition controlling whether the members are public by default or not.

225

__typeof doesn't work on older compilers. Bde will give you 10k words on this if you commit it.

After fixing the three examples I gave, a buildworld succeeds with my suggested alternative.

Hi,

Will your suggestion work with external "C" code in a CPP file?

--HPS

sys/sys/queue.h
163

Technically it is not needed, except these macros should only be used with classes, and that makes the compiler warn.

sys/sys/queue.h
225

I see. I'll update the patch to not use typeof, but instead inline the missing structure parts. Else it won't work with extern "C" like said before.

Change implementation so that typeof() is used with C++ compilers and the non-typeof version is used with C compilers. Should address any mentioned conserns so far.

HPS: Yes, my suggestion works with 'extern "C"'.

Note that 'extern "C"' does not mean "this is C code". If you're compiling with a C++ compiler, it's C++ code. The 'extern "C"' declaration simply affects the linkage and calling convention. That is, the types of the parameters are not included in the symbol, and it doesn't get mangled.

Update implementation as per suggestions.

Hi vangyzen,

I've updated the suggestion again. It's basically the same like was suggested by you, except the macro name is "QUEUE_TYPEOF" and it is uppercased.

--HPS

You will also need the three other header file changes from D2767.

In my experience, it is good etiquette to test the build and basic functionality before asking for a code review. Asking for a review prematurely might be wasting your reviewers' time. Also, since you're going to put your reviewers' names on the commit, they deserve a chance to review the final code, including all the trivial build fixes.

I would strongly recommend a ports exprun, since we found three places that needed a fix in the base system alone.

sys/sys/queue.h
168

Almost every occurrence of "struct type" needs to be changed to use QUEUE_TYPEOF. See D2767.

704
#undef QUEUE_TYPEOF

to avoid namespace pollution.

Patch some missed "struct type" to macro conversions.

sys/sys/queue.h
168

Right - fixed.

704

It needs to be there, else the code won't compile when the macros refer QUEUE_TYPEOF().

Import some missing pieces from D2767 .

Due to a lot of package fallouts declare separate HEAD and ENTRY macros for classes. Else patch remains the same.

Added manual page update after adding new macros.

brueffer added inline comments.
share/man/man3/Makefile
69 ↗(On Diff #6355)

These new entries should be inserted by alphabetical order (the present entries are already sorted that way).

share/man/man3/queue.3
37 ↗(On Diff #6355)

Same here, please keep the sorting.

I have to do the exp-run again, the previous patch wasn't tested as it wasn't relative to src tree

hselasky edited edge metadata.

Update patch as per comments.

wblock added inline comments.
share/man/man3/queue.3
341 ↗(On Diff #6358)

This sentence is really two different sentences. Removing the comma after "class" helps, but it is still too complicated. Better to split it:

In the macro definitions,
​.Fa CLASSTYPE
​is the name of a user defined class.
The class must contain a field called
.Fa NAME
which is of type
.Li SLIST_CLASS_ENTRY ,
​.Li STAILQ_CLASS_ENTRY ,
​.Li LIST_CLASS_ENTRY ,
​or
​.Li TAILQ_CLASS_ENTRY .

Implement Warren Block's comments.

wblock added a reviewer: wblock.

Man page looks good, accepted (man page only). It never hurts to run igor and mandoc -Tlint on a man page, though. Thanks!

This revision is now accepted and ready to land.Jun 23 2015, 11:27 PM