CDRIVER-6085 Reorder + annotate include directives with "IWYU pragma: export" #2167
Conversation
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
Apologies for late review. The efforts towards include hygiene are appreciated.
| #include <stdbool.h> | ||
| #include <stdint.h> | ||
| #include <limits.h> // IWYU pragma: export | ||
| #include <stdarg.h> // IWYU pragma: export |
There was a problem hiding this comment.
Remove unneeded export pragma? Later code references va_copy:
#if !defined(va_copy) && defined(__va_copy)
There was a problem hiding this comment.
As a compatibility header, this header conditionally provides va_copy when <stdarg.h> does not. Therefore, this header transitively provides <stdarg.h>.
src/libbson/src/bson/compat.h
Outdated
| #include <fcntl.h> // IWYU pragma: export | ||
| #include <sys/stat.h> // IWYU pragma: export | ||
|
|
||
| #include <ctype.h> |
There was a problem hiding this comment.
The various stdlib and system includes in
bson-compat.hwhich are currently marked as "unused" by IWYU are left as-is (to avoid potentially breaking downstream users) and do not have the IWYU export pragma applied
To check: is that still true? I do not see any warnings from clangd. However, something may be different in my local environment. Removing other IWYU pragmas in this file (e.g. limits.h, stdbool.h) does not show unused include warnings as I would expect.
There was a problem hiding this comment.
This is because much of the contents of this file were to support old pre-C99 compatibility, and despite incremental removal of pre-C89 compat features, I do not think the include directives were cleaned up accordingly.
Applied the "keep" pragma to headers currently marked as unused.
src/libbson/src/bson/bson-private.h
Outdated
| #ifndef BSON_PRIVATE_H | ||
| #define BSON_PRIVATE_H | ||
|
|
||
| #include <bson/bson.h> // IWYU pragma: export |
There was a problem hiding this comment.
Suggest removing export, since I do not think bson-private.h is the direct private analog to bson.h.
(Aside:
bson-private.hshould probably be renamed tobson-types-private.hfor consistency?)
Since bson-private.h defines the implementation types of bson_t, I would rather rename to bson_t-private.h.
There was a problem hiding this comment.
Good point. Changed the component header to <bson/bson_t.h> and renamed it to bson_t-private.h accordingly.
3 participants
Related to CDRIVER-6085. Applies IWYU export pragmas to component headers in mongo-c-driver. Companion PR to mongodb/mongo-cxx-driver#1492.
To avoid potentially breaking downstream users, no include directives are removed by changes in this PR. However, component header includes are reordered to the top, with necessary fixes applied to impacted downstream components. These include:
<bson/bson.h>: missing<bson/bson_t.h>and<bson/error.h>(see: [CDRIVER-4153] Beginning of Header Hygiene & Verification #2053)<bson/bson-vector.h>: missing<bson/bson_t.h>and<bson/bson-types.h><mongoc/mongoc-topology-description.h>: missing<mongoc/mongoc-server-description.h>.<mongoc/mongoc-stream-tls-secure-transport.h>: missing<mongoc/mongoc-ssl.h>and<mongoc/mongoc-stream.h>.(Aside:
bson-private.hshould probably be renamed tobson-types-private.hfor consistency?)Additionally, the
bson-config.hheader is exported bybson-macros.handbson-compat.h, andbson-macros.his exported bybson-compat.h. This is due to the global nature of these headers in providing system-defined entities (types, functions, macros, etc.). The various stdlib and system includes inbson-compat.hwhich are currently marked as "unused" by IWYU are left as-is (to avoid potentially breaking downstream users) and do not have the IWYU export pragma applied.Note
Although IWYU supports the "private" pragma to denote private component headers which require the separate inclusion of the corresponding public header, this PR proposes we avoid this pattern in favor of supporting standalone-includable headers whenever able, even for private headers, by ensuring the corresponding public header is first included by the private header.
With the changes in this PR, the IWYU tool (via clangd or otherwise) may be used to start properly addressing "Condition 1" in CDRIVER-6085 via missing-header and unused-header diagnostics.