-
Notifications
You must be signed in to change notification settings - Fork 192
Apply IWYU to {test/unit,examples,src}/*.{cpp,hpp}. #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| USA | ||
| */ | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| #include <httpserver.hpp> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| */ | ||
|
|
||
| #include <httpserver.hpp> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| */ | ||
|
|
||
| #include <httpserver.hpp> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| using namespace httpserver; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why here we are preferring an explicit inclusion to a slimmer forward declaration.
It actually feels opposite (or orthogonal at the very least) to the spirit of the commit. While I am in agreement with the rest of the commit (as a meant to avoid implicit transitive dependencies) I disagree with a general preference for inclusion over forward-declaration (if that is what we are trying to achieve).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My experience is that forward declarations[1] are almost never a good idea.
tl;dr; a bunch of people, who I've worked with and who's judgment I trust and respect, and who's job it is to study and understand the costs and benefits of this sort of decision say that using forward declarations doesn't actually give the benefits you are seeking.
Starting with this: https://google.github.io/styleguide/cppguide.html#Forward_Declarations
the only cited pros:
And then they list the cons, of which there are many.
If you want to cut down on includes and build times, then the biggest lever would be to allow and encourage direct use of the httpserver/*.hpp files by all clients.
That said, while I strongly think that using forward declarations provide little if any value and introduces real risks and costs, and am willing to discuss that with you in an effort to convince you[2]; this is your code. If you decide to use forward declarations then that's your decision to make and I'll respect that.
[1] that is where they are not in the header associated with the definition.
[2] there is a good snippet out of a conference talk I watched somewhere on this (but I'm not finding it) that goes over some empirical anecdata about the actual effect (or lack there of) with modern tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I agree with their cons in google styleguide. Most of their cons do not apply here (especially in the context of forward declaring within the library itself). Their pros do.
They seem to be ignoring the pollution that inclusion in C++ necessarily drags along. Also, it feels like what is happening here is that they are removing the good uses of an instrument in view of potential bad uses. In essence, I disagree with them :). I think it might make sense when managing thousands of people to make a broad draconian law, it makes more sense in this case to use the specific advantage of the tool at hand.
Taking their cons one by one:
Not sure how this could be the case as we should be using them anyhow only in case of pointers/references.
Does not apply here.
Does not apply here (it also feels hardly generalizable)
Not sure it is a con but rather a statement at root
Maybe, but it feels like a weak counter-argument against recompilation and pollution
Valid, but again it is not happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does apply here, maybe. For example if you were to need to make one of the classes into a template (with default arguments) or an alias (typedef or using) then it would be necessary to go remove or update the forward declarations. (Thought I don't know how likely that kind of change would be.)
Much of my argument is that "recompilation and pollution" is it self a weak argument: I'm tempted to see how much the transitive includes from the .cpp files change if forward declarations are aggressively used. And then how much the compile time changes. (I'm guessing not much in both cases.)
The other half of the argument is that using forward declarations introduces something, at a distance, that needs to be maintained in sync with the definition. Which is to say that it can be out of sync, and (by Murphy's law) sooner or later will.
(FWIW: there is one forward declaration that I had to leave in. It's associated with a strange cycle in the dependencies that I'm hoping to remove at some point.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see the cons applied here and hence the whole problems is falling for me in the realm of speculation. Not sure even what we need to keep in sync here given that we are not forward declaring anything but class names, so I'd rather stay close to the practicality of the problem rather than generalize so much to land in the space of philosophy.
The real problem for me is pollution (not much compilation speed). I can't see why I would want to drag this code and every piece of code depending on it to include everything included here. Moving the inclusions in the cop file allows me to free these classes and their dependents from their weight (be it bytes or noise).
Let's leave these in places and as a general policy for the lib in general I do prefer forward declarations to includes unless there is a tangible material risk associated with doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll grant it is somewhat of a hypothetical but; say for example you want to allow using multiple threading models in the same binary and chose to to that by making all the relevant classes templates. You would need to touch every relevant forward declaration. Or maybe you find a need to rename a type and to allow the old new to work (but be deprecated) you add a
using old_name = new_name;or similarly for moving something to a new namespace. Again every declaration needs to change.As for pollution: Out of curiosity I looked up the set of include from each
.cpp(by invokingcpp -M ...) in the branch vs. in the current head. With a little filtering to canonicalize things, the only addition this PR makes is to addhttpserver/details/modded_request.hppto the includes of eachexamples/*.cpp. Everything else was already included. (That said, I haven't compared that to this branch + use of forward declarations, but my guess is it won't change much.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On templatizing those classes: I promise I will not do that. They were, and are not anymore. If someone does it, they will likely do it once say in N years (not every day of the week). We will be able to survive.
On the pollution: That is not my point of pollution. If anything the test is relatively obvious because for every include missing in a hpp you are going to find it in the cpp file. I am perfectly happy at symbols being in the so (where they cost nothing as, at worst, is a matter of linking), they still aren't forcing them down in every "h" file. To me the point is that each piece of code should just know only the bare minimum it needs to know. Nothing else. The recompilation of everything that results in this case to me is just the symptom (not the cause) of the problem with it. It just means that "needless noise" has been just added to the hpp.
Again, I am talking specifically here of how this semantic is used in the cases (one by one) you are touching in which what we need is just the name of a struct/class and the speculations on its structure are minimal or close to none. For example, microhttpd.h was never included in "h" files by convention as I don't know (and don't want to know) what they include in their "h" file to be able to use a single struct as pointer and I don't want my clients to have to know that. They don't need it. We have often run exercises to convert includes in FDs with this purpose and I intend to keep doing so in the future.
I am not sure this debate is useful at this point though. I'd rather capitalize on the rest of the change that I believe it is useful than staying stuck on something I cannot be brought to care about.
I think we should just move on and leave the forward declarations as is, unless, again we have a problem with them that is real, happening right now and currently acting against us in the library (and that also would have to be specific for each use of them, because I am still unconvinced by the generality of the tool being bad).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a not so funny and practical example of what I am talking about and referring as "pollution", see what is happening here #187 where a simple movement of includes up and down is breaking a build on windows (still to root-cause why).
Especially in light of how much garbage many libraries tend to include in pre-processor, macros, etc... I'd rather not have them where it more visibly impacts everything else and our customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding including microhttpd.h from headers, if anything, that's the case I'd most want to avoid FD's as this project doesn't control that one (or are you also an author of it?) and adding FD's has your code making assumptions about it's implementation details. (What if one of those types were to be converted to a
typedefor even a macro#define?)Regarding the ordering issue you are chasing down, I come to the opposite conclusion: the goal state should be that every header used in the project should be 100% self contained, that is the order they are instantiated in shouldn't ever matter. The easy test for that is to make sure that for every header there is some .cpp file with it as the first header.
In my experience, the cases were that can't be done are rather limited. E.g.:
Other than those, I don't remember ever seeing any cases where "solving that problem" didn't improve the code in general.
Regardless which way you decide to approve, I'm going to sit on this until you have the windows build sorted out and I can place this work on top of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playing with it right now (windows I mean). I got it to build by fixing the define they need for the windows version. I noticed that they have improved the support for msys2 so trying to get it to work in a cleaner way (possibly running tests this time around). I'll ping you once done.