Page MenuHomeFreeBSD

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

Authored by hselasky on Jun 6 2015, 7:10 PM.

Details

Reviewers
wblock
vangyzen
Group Reviewers
manpages

Diff Detail

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

Event Timeline

hselasky updated this revision to Diff 5997.Jun 6 2015, 7:10 PM
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.
emaste added a subscriber: emaste.Jun 7 2015, 3:01 AM

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

hselasky updated this revision to Diff 6008.Jun 7 2015, 6:40 AM

Add full context to diff.

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?

imp added a comment.Jun 8 2015, 3:06 PM

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
165

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.

237

__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

hselasky added inline comments.Jun 9 2015, 10:07 AM
sys/sys/queue.h
165

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

hselasky added inline comments.Jun 9 2015, 10:13 AM
sys/sys/queue.h
237

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.

hselasky updated this revision to Diff 6059.Jun 9 2015, 11:21 AM

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.

I put my alternative in D2767.

hselasky updated this revision to Diff 6063.Jun 9 2015, 12:58 PM

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
175

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

752
#undef QUEUE_TYPEOF

to avoid namespace pollution.

hselasky updated this revision to Diff 6149.Jun 12 2015, 7:36 PM

Patch some missed "struct type" to macro conversions.

hselasky added inline comments.Jun 12 2015, 7:38 PM
sys/sys/queue.h
175

Right - fixed.

752

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

hselasky updated this revision to Diff 6151.Jun 12 2015, 9:32 PM

Import some missing pieces from D2767 .

hselasky updated this revision to Diff 6152.Jun 12 2015, 9:43 PM

Fix typo that sneaked in.

hselasky updated this revision to Diff 6327.Jun 19 2015, 10:34 AM

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

hselasky updated this revision to Diff 6355.Jun 20 2015, 11:33 PM

Added manual page update after adding new macros.

brueffer added inline comments.
share/man/man3/Makefile
71

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

share/man/man3/queue.3
39

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 updated this revision to Diff 6358.Jun 21 2015, 1:32 PM
hselasky edited edge metadata.

Update patch as per comments.

wblock added a subscriber: wblock.Jun 22 2015, 1:01 AM
wblock added inline comments.
share/man/man3/queue.3
341

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 .

hselasky updated this revision to Diff 6411.Jun 23 2015, 10:50 PM

Implement Warren Block's comments.

wblock accepted this revision.Jun 23 2015, 11:27 PM
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