X Tutup
Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/allowing_disallowing_methods.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
4 changes: 4 additions & 0 deletions examples/basic_authentication.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

#include <httpserver.hpp>

#include <memory>
#include <string>
#include <utility>

using namespace httpserver;

class user_pass_resource : public httpserver::http_resource
Expand Down
1 change: 1 addition & 0 deletions examples/benchmark_nodelay.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <cstdlib>
#include <memory>
#include <utility>

#include <httpserver.hpp>

Expand Down
1 change: 1 addition & 0 deletions examples/benchmark_select.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <cstdlib>
#include <memory>
#include <utility>

#include <httpserver.hpp>

Expand Down
1 change: 1 addition & 0 deletions examples/benchmark_threads.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <cstdlib>
#include <memory>
#include <utility>

#include <httpserver.hpp>

Expand Down
3 changes: 3 additions & 0 deletions examples/custom_access_log.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
*/

#include <iostream>
#include <memory>
#include <string>
#include <utility>

#include <httpserver.hpp>

Expand Down
3 changes: 3 additions & 0 deletions examples/custom_error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
5 changes: 5 additions & 0 deletions examples/deferred_with_accumulator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,13 @@
USA
*/

#include <string.h>
#include <sys/types.h>
#include <algorithm>
#include <atomic>
#include <chrono>
#include <memory>
#include <string>
#include <thread>

#include <httpserver.hpp>
Expand Down
4 changes: 4 additions & 0 deletions examples/digest_authentication.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
USA
*/

#include <memory>
#include <string>
#include <utility>

#include <httpserver.hpp>

#define MY_OPAQUE "11733b200778ce33060f31c9af70a870ba96ddd4"
Expand Down
3 changes: 3 additions & 0 deletions examples/handlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
4 changes: 4 additions & 0 deletions examples/hello_with_get_arg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
USA
*/

#include <memory>
#include <string>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
3 changes: 3 additions & 0 deletions examples/hello_world.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
*/

#include <iostream>
#include <memory>
#include <string>
#include <utility>

#include <httpserver.hpp>

Expand Down
6 changes: 6 additions & 0 deletions examples/minimal_deferred.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
USA
*/

#include <string.h>
#include <sys/types.h>
#include <memory>
#include <string>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
3 changes: 3 additions & 0 deletions examples/minimal_file_response.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
3 changes: 3 additions & 0 deletions examples/minimal_hello_world.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
3 changes: 3 additions & 0 deletions examples/minimal_https.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
USA
*/

#include <memory>
#include <utility>

#include <httpserver.hpp>

using namespace httpserver;
Expand Down
2 changes: 2 additions & 0 deletions examples/minimal_ip_ban.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
*/

#include <httpserver.hpp>
#include <memory>
#include <utility>

using namespace httpserver;

Expand Down
4 changes: 3 additions & 1 deletion examples/service.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
USA
*/

#include <stdint.h>
#include <stdlib.h>
#include <unistd.h>

#include <cstdio>
#include <iostream>
#include <memory>

#include <httpserver.hpp>

Expand Down
2 changes: 2 additions & 0 deletions examples/setting_headers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
*/

#include <httpserver.hpp>
#include <memory>
#include <utility>

using namespace httpserver;

Expand Down
3 changes: 3 additions & 0 deletions examples/url_registration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
*/

#include <httpserver.hpp>
#include <memory>
#include <string>
#include <utility>

using namespace httpserver;

Expand Down
6 changes: 5 additions & 1 deletion src/details/http_endpoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@

#include "httpserver/details/http_endpoint.hpp"

#include <stddef.h>
#include <cctype>
#include <memory>
#include <stdexcept>

#include "httpserver/http_utils.hpp"
#include "httpserver/string_utilities.hpp"

using namespace std;

Expand Down
3 changes: 3 additions & 0 deletions src/file_response.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
#include "httpserver/file_response.hpp"

#include <fcntl.h>
#include <microhttpd.h>
#include <stddef.h>
#include <unistd.h>

using namespace std;

Expand Down
3 changes: 3 additions & 0 deletions src/http_request.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

#include "httpserver/http_request.hpp"

#include <microhttpd.h>
#include <stdio.h>
#include <stdlib.h>
#include <iostream>

#include "httpserver/http_utils.hpp"
Expand Down
5 changes: 1 addition & 4 deletions src/http_resource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@

#include "httpserver/http_resource.hpp"

#include <stdlib.h>
#include <microhttpd.h>

#include "httpserver/http_request.hpp"
#include "httpserver/http_response.hpp"
#include "httpserver/http_utils.hpp"
#include "httpserver/string_response.hpp"
#include "httpserver/string_utilities.hpp"
#include "httpserver/webserver.hpp"

using namespace std;

Expand Down
8 changes: 2 additions & 6 deletions src/http_response.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,11 @@

#include "httpserver/http_response.hpp"

#include <fcntl.h>
#include <unistd.h>
#include <cstdio>
#include <functional>
#include <microhttpd.h>
#include <iostream>
#include <sstream>
#include <utility>

#include "httpserver/http_utils.hpp"
#include "httpserver/webserver.hpp"

using namespace std;

Expand Down
17 changes: 10 additions & 7 deletions src/http_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,25 @@
#include "httpserver/http_utils.hpp"

#if defined(__MINGW32__) || defined(__CYGWIN32__)
#include <ws2def.h>
#include <winsock2.h>
#include <ws2tcpip.h>
#else
#include <sys/socket.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <netdb.h>
#include <netinet/in.h>
#include <sys/socket.h>
#include <sys/types.h>
#endif

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <iomanip>
#include <fstream>
#include <iostream>
#include <iomanip>
#include <memory>
#include <sstream>
#include <stdexcept>
#include <utility>

#include "httpserver/string_utilities.hpp"

Expand Down Expand Up @@ -404,7 +407,7 @@ ip_representation::ip_representation(const std::string& ip)

if (parts[i].size() < 4)
{
stringstream ss;
std::stringstream ss;
ss << setfill('0') << setw(4) << parts[i];
parts[i] = ss.str();
}
Expand Down Expand Up @@ -546,7 +549,7 @@ bool ip_representation::operator <(const ip_representation& b) const

const std::string load_file (const std::string& filename)
{
ifstream fp(filename.c_str(), ios::in | ios::binary | ios::ate);
std::ifstream fp(filename.c_str(), ios::in | ios::binary | ios::ate);
if(fp.is_open())
{
std::string content;
Expand Down
3 changes: 3 additions & 0 deletions src/httpserver.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@

#define _HTTPSERVER_HPP_INSIDE_

// IWYU pragma: begin_exports
#include "httpserver/basic_auth_fail_response.hpp"
#include "httpserver/create_webserver.hpp"
#include "httpserver/deferred_response.hpp"
#include "httpserver/digest_auth_fail_response.hpp"
#include "httpserver/file_response.hpp"
Expand All @@ -33,5 +35,6 @@
#include "httpserver/http_utils.hpp"
#include "httpserver/string_response.hpp"
#include "httpserver/webserver.hpp"
// IWYU pragma: end_exports

#endif
4 changes: 4 additions & 0 deletions src/httpserver/basic_auth_fail_response.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
#ifndef _BASIC_AUTH_FAIL_RESPONSE_HPP_
#define _BASIC_AUTH_FAIL_RESPONSE_HPP_

#include <microhttpd.h>
#include <string>

#include "httpserver/http_utils.hpp"
#include "httpserver/string_response.hpp"

namespace httpserver
Expand Down
7 changes: 4 additions & 3 deletions src/httpserver/create_webserver.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@
#ifndef _CREATE_WEBSERVER_HPP_
#define _CREATE_WEBSERVER_HPP_

#include <stdint.h>
#include <stdlib.h>
#include <string>
#include <memory>

#include "httpserver/http_request.hpp"
#include "httpserver/http_response.hpp"
#include "httpserver/http_utils.hpp"

Expand All @@ -35,9 +39,6 @@

namespace httpserver {

class webserver;
class http_request;
Comment on lines -38 to -39
Copy link
Copy Markdown
Owner

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).

Copy link
Copy Markdown
Contributor Author

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:

  • avoid reading more headers. But in general that's doesn't happen, as very often the header ends up being included anyway in the translation unit. Also the full build time for everything here is maybe 30 seconds, given any effort to run a parallel build, that advantage vanishes.
  • Avoid extra compiles. Again, that's dubious for the same reason.

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.

Copy link
Copy Markdown
Owner

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:

Forward declarations can hide a dependency, allowing user code to skip necessary recompilation when headers change.

Not sure how this could be the case as we should be using them anyhow only in case of pointers/references.

A forward declaration may be broken by subsequent changes to the library. Forward declarations of functions and templates can prevent the header owners from making otherwise-compatible changes to their APIs, such as widening a parameter type, adding a template parameter with a default value, or migrating to a new namespace.

Does not apply here.

Forward declaring symbols from namespace std:: yields undefined behavior.

Does not apply here (it also feels hardly generalizable)

It can be difficult to determine whether a forward declaration or a full #include is needed.

Not sure it is a con but rather a statement at root

Forward declaring multiple symbols from a header can be more verbose than simply #includeing the header.

Maybe, but it feels like a weak counter-argument against recompilation and pollution

Structuring code to enable forward declarations (e.g. using pointer members instead of object members) can make the code slower and more complex.

Valid, but again it is not happening here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forward declaration may be broken by subsequent changes to the library.

Does not apply here.

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.)

Forward declaring multiple symbols from a header can be more verbose ...

Maybe, but it feels like a weak counter-argument against recompilation and pollution

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.)

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
Contributor Author

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 invoking cpp -M ...) in the branch vs. in the current head. With a little filtering to canonicalize things, the only addition this PR makes is to add httpserver/details/modded_request.hpp to the includes of each examples/*.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.)

for f in $(find -name '*.cpp' | sort);
do
  echo $(\
      cpp -I${libmicrohttpd_include} -Itest -Isrc \
        -DHTTPSERVER_COMPILATION  \
        -M $f | \
      sed 's| /[^ ]*||g;s/\b \b/\n /g;s/ \\$//;s/.*://' | \
      sort -u
  );
done

Copy link
Copy Markdown
Owner

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).

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
Contributor Author

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 typedef or 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.:

  • Some external header refuses to cooperate (like the windows stuff, in which case I'd wrap it in my own header that hides the issue away).
  • "textual headers" (I've seen ones that are specifically intended to be included multiple times in the same translation unit, and not necessarily from global scope, which I consider a bad idea.)

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.

Copy link
Copy Markdown
Owner

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.


typedef const std::shared_ptr<http_response>(*render_ptr)(const http_request&);
typedef bool(*validator_ptr)(const std::string&);
typedef void(*log_access_ptr)(const std::string&);
Expand Down
6 changes: 6 additions & 0 deletions src/httpserver/deferred_response.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@
#ifndef _DEFERRED_RESPONSE_HPP_
#define _DEFERRED_RESPONSE_HPP_

#include <microhttpd.h>
#include <stddef.h>
#include <stdint.h>
#include <sys/types.h>
#include <memory>
#include <string>

#include "httpserver/string_response.hpp"
#include "httpserver/http_utils.hpp"

namespace httpserver
{
Expand Down
Loading
X Tutup