Include what you use #167
Include what you use #167
Conversation
There are a couple of things to call out here: 1. I have done this naively. We may want consumers of this API to only have to include <transport/transport.h> to get all/some subset of the <transport/*.h> headers. We can add a pragma to mark those as exported, that consumers of the API and ourselves will understand if they also run clang-tidy or IWYU. 2. On MacOS at least, IncludeCleaner suggests some private MacOS headers that contain the actual implementations rather than the more canonical UNIX headers. E.g timeval should come from sys/time.h not <sys/types/_timeval.h>. IWYU allows a mapping file to correct these, but clang-tidy's version doesn't seem to. I have marked them NOLINT for now.
RichLogan
force-pushed
the
missing-includes
branch
from
e906fa3 to
70373ac
Compare
| @@ -3,6 +3,10 @@ | |||
| #if not defined(PLATFORM_ESP) | |||
| #include <transport_picoquic.h> | |||
| #endif | |||
| #include <memory> | |||
| #include <stdexcept> | |||
| #include <spdlog/logger.h> | |||
There was a problem hiding this comment.
It preferred this over
#include <spdlog/spdlog.h>?
There was a problem hiding this comment.
Yeah, it doesn't know about umbrella headers that don't actually define the thing directly, although we can declare them in a mapping file so it does know. Here it didn't seem too horrendous so I didn't bother. But that's the same thing that I'm raising in my comment about <transport/*.h>. We can add a pragma in that case if we want.
| // PicoQuic. | ||
| #include <picoquic.h> | ||
| #include <picoquic_internal.h> | ||
| #include <picoquic_packet_loop.h> | ||
| #include <autoqlog.h> | ||
| #include <picoquic_utils.h> | ||
| #include <spdlog/spdlog.h> | ||
|
|
||
| #include <picoquic_config.h> | ||
| #include <picosocks.h> | ||
|
|
||
| // Transport. | ||
| #include "transport/transport.h" | ||
| #include "transport/transport_metrics.h" | ||
| #include "transport/safe_queue.h" | ||
| #include "transport/stream_buffer.h" | ||
| #include "transport/priority_queue.h" | ||
| #include "transport/span.h" | ||
| #include "transport/time_queue.h" | ||
| #include <spdlog/logger.h> | ||
|
|
||
| // System. | ||
| #include <arpa/inet.h> | ||
| #include <sys/socket.h> | ||
| #include <cassert> | ||
| #include <cstring> | ||
| #include <ctime> | ||
| #include <iostream> | ||
| #include <thread> | ||
| #include <unistd.h> | ||
| #include <sstream> | ||
| #include <cstdint> | ||
| #include <optional> | ||
| #include <utility> | ||
| #include <memory> | ||
| #include <mutex> | ||
| #include <vector> | ||
| #include <string> | ||
| #include <chrono> | ||
| #include <sstream> | ||
| #include <stdexcept> |
There was a problem hiding this comment.
Should we run the formatter over this to sort them?
There was a problem hiding this comment.
Also, personally (though I'm sure I got this habit from somewhere vaguely official) I like to order the headers as
#include "local/header.h"
#include <dependency/header.h>
#include <system/header>That way your dependency headers are always explicitly after your locally declared headers so that you don't run into potential include errors where you didn't include a dependency header in a local header, but that gets hidden when the dependency header comes before the local header in this implementation file. Of course IWYU fixes that issue.
There was a problem hiding this comment.
Yeah these will all get reordered when we run a format pass (#159) , and I think that's the order it'll do them it.
Google suggests order of:
Related header, C system headers, C++ standard library headers, other libraries' headers, your project's headers.
| @@ -821,7 +842,7 @@ PicoQuicTransport::ConnectionContext& PicoQuicTransport::createConnContext(picoq | |||
| /*(const void*)(&((struct sockaddr_in*)addr)->sin_addr),*/ | |||
| conn_ctx.peer_addr_text, | |||
| sizeof(conn_ctx.peer_addr_text)); | |||
| conn_ctx.peer_port = ntohs(((struct sockaddr_in*)addr)->sin_port); | |||
| conn_ctx.peer_port = ntohs(((struct sockaddr_in*)addr)->sin_port); // NOLINT (include) | |||
There was a problem hiding this comment.
What's the linter mad about?
There was a problem hiding this comment.
nthos is declared in a weird implementation header on Mac at least, so it's asking me to include that rather than the canonical header we want. I can also fix this in a mapping file.
| @@ -485,7 +504,7 @@ bool UDPTransport::send_data(ConnectionContext& conn, DataContext& data_ctx, con | |||
| * - loop reads data from fd_write_queue and writes it to the socket | |||
| */ | |||
| void UDPTransport::fd_writer() { | |||
| timeval to; | |||
| timeval to; // NOLINT (include). | |||
There was a problem hiding this comment.
Oh boy, that's a lot of these comments. How do we make it happy?
There are a couple of things to call out here:
<transport/transport.h>to get all/some subset of the<transport/*.h>headers. We can add a pragma to mark those as exported that will be understood if they/us run clang-tidy or IWYU. Perhaps the existing ones transport includes already would make sense(?): cc @TimEvenstimevalshould come fromsys/time.hnotsys/types/_timeval.h. IWYU allows a mapping file to correct these, but clang-tidy's version doesn't seem to. I have marked them NOLINT for now.