For “normal” symbols in a file we try to avoid listing them next to the header that makes them available in the file. However, for identifiers used in a #if or #ifdef directive, it could make sense to list them. Especially for config/bitcoin-config.h.
Example:
From #26924 (comment)
It’d be really nice if we could avoid including bitcoin-config.h from headers at all, that would pretty much solve the problem.
But even simple examples like this one make that difficult without reorganizing code (and this kind of thing shouldn’t dictate code imo) :(
But even simple examples like this one make that difficult without reorganizing code (and this kind of thing shouldn’t dictate code imo) :(
That example works without config.h or configure at all, __SIZEOF_INT128__ is set by the compiler.
@sipa Huh, thanks.
I could’ve sworn I saw an autoconf check for that that defined it when I was grepping around last week, but I guess I got it mixed up in my head with another. The lack of HAVE_ or USE_ should’ve been a giveaway :)
Edit: Ah, I guess I just assumed that’s what the #include bitcoin-config.h in that file was for as it’s the only define that gets checked there.
I’ve pushed a branch here that does a large cleanup: https://github.com/theuni/bitcoin/commits/cleanup-config-h-headers/
The approach is to include bitcoin-config.h in any file which uses a configure define, even if it would have been included by an earlier header. That gives us a safe starting point for further removals.
My approach was somewhat manual, but I think it’s trustworthy:
Grab all possibly defined tokens: grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|'
That leaves us with:
AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES
That’s what I used for grepping with git grep -E.
I then filtered out those which already included bitcoin-config.h and added where necessary.
Finally, with all files directly including bitcoin-config.h as necessary, I used a few combinations of git grep -L -E <tokens> to find all files which don’t require bitcoin-config.h, and used that in a git grep -l bitcoin-config.h -- <those files> to find the files that don’t require the include.
Hence, the branch above should have the following properties:
@sipa Thanks.
@maflcko If we should decide to list the used identifiers for each bitcoin-config.h include as you’ve suggested here (which I’m not sure is necessary, but not opposed to either), we’d need to make sure to include the file where needed in each instance.
So I believe something like the above branch would be a requirement. And since @sipa and @hebasto agree, I think it makes sense to go ahead and do that either way.
I’ll open a PR for that branch with instructions to reproduce.
I used a few combinations of git grep -L -E to find all files which don’t require bitcoin-config.h, and used that in a git grep -l bitcoin-config.h – to find the files that don’t require the include.
Iwyu should be able to find includes that are present, but not needed. So ideally, your result should match what iwyu finds.
While iwyu can find some missing includes, it can’t find all. So I’ve written a lint check in #29408
Closing for now, let’s continue the discussion in the two pull requests.