Header cleanup guided by IWYU #2902
Conversation
vittorioromeo
requested review from
Bromeon,
eXpl0it3r,
binary1248 and
ChrisThrasher
vittorioromeo
force-pushed
the
feature/header_cleanup_iwyu
branch
from
9b5d842
to
10e3b95
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2902 +/- ##
==========================================
- Coverage 44.13% 44.12% -0.01%
==========================================
Files 254 254
Lines 21288 21288
Branches 5214 5213 -1
==========================================
- Hits 9395 9394 -1
- Misses 10886 10908 +22
+ Partials 1007 986 -21
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
vittorioromeo
force-pushed
the
feature/header_cleanup_iwyu
branch
2 times, most recently
from
2bd6335
to
5ff260b
Compare
| m_impl->maxSocket = std::max(m_impl->maxSocket, handle); | ||
| if (m_impl->maxSocket < handle) | ||
| m_impl->maxSocket = handle; |
There was a problem hiding this comment.
Would be nice to not mix unrelated changes in with tool-guided refactors, no matter how small they seem.
There was a problem hiding this comment.
Sorry, I made this change because std::max was under a Unix-only #ifdef, and <algorithm> was only being used for Unix builds. Between the choice of putting <algorithm> itself into a #ifdef, keeping <algorithm> included regardless of the platform (which triggers the tool), and changing the single occurrence of std::max to an if, I chose the latter as it seemed the most pragmatic solution.
I can revert this change if needed, but it is a change that was guided by IWYU.
There was a problem hiding this comment.
I’d rather use a built in standard library feature that reimplement it to just to remove a single header.
There was a problem hiding this comment.
I’d rather use a built in standard library feature that reimplement it to just to remove a single header.
I generally agree with your sentiment, however in this particular situation it seems overkill to include the entirety of <algorithm> (fairly heavyweight header) just for a single call to std::max which is conditionally compiled only when building for Unix platforms.
The simplest and most pragmatic choice between...
- Include the entirety of
<algorithm>unconditionally and usestd::maxonce; - Include
<algorithm>conditionally only when building for Unix and usestd::maxonce; - Use a
ifstatement
...in this particular situation is IMHO (3).
You say "just to remove a single header", but the opposite perspective is "include an entire header just to call a single std::max" :)
Regardless, if you are going to block the PR's approval on this, I will revert the change and re-propose it as a separate PR.
There was a problem hiding this comment.
You say "just to remove a single header", but the opposite perspective is "include an entire header just to call a single
std::max" :)
I think it's unfortunate that the standard library didn't compartmentalize headers better, but at some point I also think this discussion becomes unproductive. There's always one more minor thing we can do on this front 😉
How much do we want to reinvent the wheel just to save some dependencies? If max etc. are recurring so often that avoiding <algorithm> has a measureable compile-time impact, maybe it's worth writing our own header with them.
Another pragmatic approach would also be that we rely more on pre-compiled headers, under which these are all non-issues.
There was a problem hiding this comment.
How much do we want to reinvent the wheel just to save some dependencies? If max etc. are recurring so often that avoiding has a measureable compile-time impact, maybe it's worth writing our own header with them.
This was my preferred way forward that I actually suggested when I began improving compilation times more than a year (two years?) ago -- the proposal was shut down in favour of selectively replacing some uses of std::* directly in the source file. You can see this in Rect<T>::contains and Rect<T>::findIntersection. In retrospect, maybe I shouldn't have caved in so easily... :)
I'd be happy to go down this route and change existing code in a separate PR, if everyone agrees.
I think it's unfortunate that the standard library didn't compartmentalize headers better, but at some point I also think this discussion becomes unproductive. There's always one more minor thing we can do on this front 😉
"didn't compartmentalize headers better" is the understatement of the century :D
After preprocessing, <algorithm> is around 10000 lines of code.
Code reuse and deduplication has both value and costs. At one point, one must genuinely ask: is it worth introducing a dependency that carries over 10000 lines of code just for a single use of std::max that is conditionally compiled only for Unix platforms?
I sincerely believe that the answer is a plain "no".
Had it been 5-6 usages of std::max and std::min, then it would have been more difficult to make this decision. Maybe reimplementing std::max and std::min in the source file would have made more sense, or maybe keeping <algorithm> was the lesser evil.
Remember, std::max is... an if statement. I'm sure we can get that right :)
Had we used something like std::remove_if, or std::rotate, or std::adjacent_find... at that point the chance of making a mistake in the implementation and the burden of reviewing it outweighs the negatives of including <algorithm>.
Every decision we make has pros/cons... I don't think it's good engineering to say "let's use the standard library wherever possible", because using the standard library has costs and drawbacks. We should use the standard library when the benefits outweighs costs and drawbacks, and I think this change in this PR is a clear example where using <algorithm> to replace a single trivial if statement was overkill.
Another pragmatic approach would also be that we rely more on pre-compiled headers, under which these are all non-issues.
I want to get there eventually (see #2901), but it is not easy. If we get to a point where PCH=1 becomes the default, and works on every platform and every build mode, then we can start relying on it more, and not just as an optimization on some platforms.
Quite reliable, but sometimes it is aggressive and there are false positives. This is why I manually made the adjustments by looking at the tool's suggestions, I didn't blindly do what the tool said. |
vittorioromeo
force-pushed
the
feature/header_cleanup_iwyu
branch
from
5ff260b
to
96ed6fb
Compare
Mostly fixed related to transitive include reliance, with a few opportunities to replace a
#includewith a forward declaration.All changes done and checked manually, but guided by
include-what-you-use.