New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Header file cleanup for C++20 header-units #18221
Conversation
C++20 adds 'header units' as a stepping-stone to modules. Header units are regular header-files that have a 'self-contained' property -- they do not require previously-included headers to provide typedefs and what not. This addresses 2 problems discovered when using clang modules (as a proxy for C++20 header-units). a) Some headers that pay attention to OPENSSL_NO_STDIO to determine whether to declare certain FILE*-taking functions do not #include <stdio.h> themselves, relying on their includer already having done that. That breaks the above mentioned encapuslation requirement. Fixed by conditionally including stdio.h in those headers. I chose to always include stdio.h in such headers, even when they included another such header that transitively included stdio. That way they do not rely on an artifact of that intermediate header's behaviour. b) Some headers have #includes inside 'extern "C" { ... }' regions. That has a bad code-smell, but GCC and clang have extensions to permit it with implementation-defined effects. Clang needs annotation on the included files to know that they themselves are entirely inside a similar region. GCC behavesq as-if there's an extern "C++" region wrapping the included header (which must therefore wrap its contents in extern "C", if that is what it wants. In effect the includer's extern "C" region is just misleading. I didn't audit all the headers for this, only those I noticed when addressing #a. \#a is necessary to build the headers as a set of clang-modules. #b is not necessary, but as I mentioned, avoids potentially implementation-defined behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always appreciate the independence of header files.
oh, should have said, pretty sure I don't have write access to do the actual merge -- I don't expect to be making many contributions. |
The project's committers are the only people who can merge. One of them will process this in due course. |
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
Merged, thanks for the fixes. |
C++20 adds 'header units' as a stepping-stone to modules. Header units are regular header-files that have a 'self-contained' property -- they do not require previously-included headers to provide typedefs and what not. This addresses 2 problems discovered when using clang modules (as a proxy for C++20 header-units). a) Some headers that pay attention to OPENSSL_NO_STDIO to determine whether to declare certain FILE*-taking functions do not #include <stdio.h> themselves, relying on their includer already having done that. That breaks the above mentioned encapuslation requirement. Fixed by conditionally including stdio.h in those headers. I chose to always include stdio.h in such headers, even when they included another such header that transitively included stdio. That way they do not rely on an artifact of that intermediate header's behaviour. b) Some headers have #includes inside 'extern "C" { ... }' regions. That has a bad code-smell, but GCC and clang have extensions to permit it with implementation-defined effects. Clang needs annotation on the included files to know that they themselves are entirely inside a similar region. GCC behavesq as-if there's an extern "C++" region wrapping the included header (which must therefore wrap its contents in extern "C", if that is what it wants. In effect the includer's extern "C" region is just misleading. I didn't audit all the headers for this, only those I noticed when addressing #a. \#a is necessary to build the headers as a set of clang-modules. #b is not necessary, but as I mentioned, avoids potentially implementation-defined behaviour. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #18221)
C++20 adds 'header units' as a stepping-stone to modules. Header units are regular header-files that have a 'self-contained' property -- they do not require previously-included headers to provide typedefs and what not. This addresses 2 problems discovered when using clang modules (as a proxy for C++20 header-units). a) Some headers that pay attention to OPENSSL_NO_STDIO to determine whether to declare certain FILE*-taking functions do not #include <stdio.h> themselves, relying on their includer already having done that. That breaks the above mentioned encapuslation requirement. Fixed by conditionally including stdio.h in those headers. I chose to always include stdio.h in such headers, even when they included another such header that transitively included stdio. That way they do not rely on an artifact of that intermediate header's behaviour. b) Some headers have #includes inside 'extern "C" { ... }' regions. That has a bad code-smell, but GCC and clang have extensions to permit it with implementation-defined effects. Clang needs annotation on the included files to know that they themselves are entirely inside a similar region. GCC behavesq as-if there's an extern "C++" region wrapping the included header (which must therefore wrap its contents in extern "C", if that is what it wants. In effect the includer's extern "C" region is just misleading. I didn't audit all the headers for this, only those I noticed when addressing #a. \#a is necessary to build the headers as a set of clang-modules. #b is not necessary, but as I mentioned, avoids potentially implementation-defined behaviour. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#18221) (cherry picked from commit eab9dbb)
C++20 adds 'header units' as a stepping-stone to modules. Header units are regular header-files that have a 'self-contained' property -- they do not require previously-included headers to provide typedefs and what not. This addresses 2 problems discovered when using clang modules (as a proxy for C++20 header-units). a) Some headers that pay attention to OPENSSL_NO_STDIO to determine whether to declare certain FILE*-taking functions do not #include <stdio.h> themselves, relying on their includer already having done that. That breaks the above mentioned encapuslation requirement. Fixed by conditionally including stdio.h in those headers. I chose to always include stdio.h in such headers, even when they included another such header that transitively included stdio. That way they do not rely on an artifact of that intermediate header's behaviour. b) Some headers have #includes inside 'extern "C" { ... }' regions. That has a bad code-smell, but GCC and clang have extensions to permit it with implementation-defined effects. Clang needs annotation on the included files to know that they themselves are entirely inside a similar region. GCC behavesq as-if there's an extern "C++" region wrapping the included header (which must therefore wrap its contents in extern "C", if that is what it wants. In effect the includer's extern "C" region is just misleading. I didn't audit all the headers for this, only those I noticed when addressing #a. \#a is necessary to build the headers as a set of clang-modules. #b is not necessary, but as I mentioned, avoids potentially implementation-defined behaviour. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #18221) (cherry picked from commit eab9dbb)
C++20 adds 'header units' as a stepping-stone to modules. Header units are regular header-files that have a 'self-contained' property -- essentially they do not require previously-included headers to provide typedefs and what not.
This addresses 2 problems discovered when using clang modules (as a proxy for C++20 header-units).
a) Some headers that pay attention to OPENSSL_NO_STDIO to determine whether to declare certain FILE*-taking functions do not #include <stdio.h> themselves, relying on their includer already having done that. That breaks the above mentioned encapuslation requirement. Fixed by conditionally including stdio.h in those headers. I chose to always include stdio.h in such headers, even when they included another such header that transitively included stdio. That way they do not rely on an artifact of that intermediate header's behaviour.
b) Some headers have #includes inside 'extern "C" { ... }' regions. That has a bad code-smell, but GCC and clang have extensions to permit it with implementation-defined effects. Clang needs annotation on the included files to know that they themselves are entirely inside a similar region. GCC behaves as-if there's an extern "C++" region wrapping the included header (which must therefore wrap its contents in extern "C", if that is what it wants. In effect the includer's extern "C" region is just misleading. I didn't audit all the headers for this, only those I noticed when addressing #a.
#a is necessary to build the headers as a set of clang-modules. #b is not necessary, but as I mentioned, avoids potential
implementation-defined behaviour.
[Contribution assignment is now on file]