Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Please upload diffs with full context, e.g. via git diff -U99999 or svn diff --diff-cmd=diff -x -U999999
I'm hardly an expert on the queue macros - or C++ - but this change seems reasonable to me. I assume `make universe' works?
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 | ||
|---|---|---|
| 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.
| 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. | |
| 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. | |
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.
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. | |
Due to a lot of package fallouts declare separate HEAD and ENTRY macros for classes. Else patch remains the same.
I have to do the exp-run again, the previous patch wasn't tested as it wasn't relative to src tree
| 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, | |
Man page looks good, accepted (man page only). It never hurts to run igor and mandoc -Tlint on a man page, though. Thanks!