Conversation
|
Hi @tchernobog, thanks for the PR! Would you mind updating it so the include order isn't changed please? I like to keep PRs small and focused. I'm not very convinced the include order makes a difference but happy to accept a separate PR for that. But please also update the linting rules so CI doesn't break. |
Thanks for the speedy review! I split down the change in smaller commits so that it is easier to validate. I tried without changing the include order and I had several build failures. I tried to explain why the different include order makes sense in the commit message. I can of course roll back the change if you really want to (just say the word), but I would personally really recommend to go with this different ordering as it exposed a few bugs and can be very helpful to prevent them in the future. I also added a The CMake version change now is in its own commit. |
|
Thanks for explaining the include order, makes sense! I'm copying the commit message from 25b05d9 to here so it's' easier to reference.
This has highlighted more issues with includes. Could you kindly add those missing headers as well please @tchernobog? |
The change makes it easier to identify missing includes,
as before a system header included in an implementation
file might have hid a dependency in a file included later.
Consider for instance:
header.h:
... uses std::runtime_error ...
header.cpp:
#include <stdexcept>
#include "header.h"
Compiling this library would be fine, as std::runtime_error
would include stdexcept before header.h. However, if header.h
is used externally by users of this library, this include order
would have hid the fact that you need to include stdexcept in
the header too.
This lead to compilation errors for users of this library
unless they manually added the missing includes.
Switching the include order makes such errors more visible
to the library developer.
We reformat all source files via clang-format-16 to adjust to a change in include file ordering constraints. We also add a couple of missing includes that were discovered due to the change.
uatuko
left a comment
There was a problem hiding this comment.
Unsure why bin/bench-results.js is changed but thanks for sorting the headers!
While attempting to compile sources generated with grpcxx,
or the project itself when using GCC, I encountered some issues
about missing standard include headers.
I added the missing includes, and I also inverted the include order
so that local includes are always
present before system ones. This minimizes
the possibility that transitive dependencies
can silently satisfy an include requirement just
due to another local header carrying the dependency over.
Please test the Asio compilation before merging, as I am on Debian
12 and as such my version of boost is still 1.74.