WIP - Rewrite ESP8266WebServer using templates #4912
Conversation
Refactor the three versions of ESP8266WebServer and ServerSecure to a single instance of a templated class. Use typedefs to enable old, non-templated names to be used (no user changes required).
|
Argh, so it's not quite so simple. The examples where a single WebServer is instantiated work fine, but it looks like I'm now in template hell and need to template-ize ESP8266HTTPUpdateServer since it takes a pointer to a WebServer object which, before, could be an overloaded object w/virtual methods so it would "just work." Do you see any way out of it, @devyte, beyond pushing templates further up the stack? |
|
I'm reviewing this now, and IT'S PRETTY GOOD! I do have some style comments and minor changes here and there, but so far everything is minor. There is more work to do as well (e.g.: remove virtualness of RequestHandler), but that's tangential to the purpose of this PR.
Nope, that's the way this works: virtualness contaminates everywhere, so does templateness, and the two language features don't really like each other much*. There is a reasoning behind this: consistency. Either you care about runtime (virtualness), or you care about compile time (templateness), but not both. If you find yourself needing both, it's usually a good indicator of a design issue. *I'm being dramatic, of course, there are some cases where both can be used, but overall it's true: you pick one of the two philosophies, and stick with it for your design. |
|
Again, I'm at a place where SPIFFS::FS and SD::FS are incompatible and causing havoc with SDWebServer.ino. Looking through the examples, I don't see why |
| _hostHeader = headerValue; | ||
| } | ||
| } | ||
|
|
||
| if (!isForm){ | ||
| size_t plainLength; | ||
| char* plainBuf = readBytesWithTimeout(client, contentLength, plainLength, HTTP_MAX_POST_WAIT); | ||
| char* plainBuf = readBytesWithTimeout<ServerClass, ClientClass>(client, contentLength, plainLength, HTTP_MAX_POST_WAIT); |
There was a problem hiding this comment.
I think that the ServerClass (=>ServerType) template parameter isn't needed for this function. Therefore, the template arguments in the function call could be completely omitted, and the compiler could deduce the ClientType parameter from the arguments automatically. I.e.:
declare like this:
template <typename ClientType>
static char* readBytesWithTimeout(ClientType& client, size_t maxLength, size_t& dataLength, int timeout_ms)
{
...
call like this:
char* plainBuf = readBytesWithTimeout(client, contentLength, plainLength, HTTP_MAX_POST_WAIT);
There was a problem hiding this comment.
Per prior comment, for sanity I'd like to leave <ServerType, ClientType> as all template params. With that, I do need to specify the exact template params here.
| @@ -1,13 +1,13 @@ | |||
| #ifndef REQUESTHANDLER_H | |||
| #define REQUESTHANDLER_H | |||
|
|
|||
| class RequestHandler { | |||
| template<class ServerClass, class ClientClass> class RequestHandler { | |||
| public: | |||
| virtual ~RequestHandler() { } | |||
There was a problem hiding this comment.
Now that RequestHandler is a templated class, this needs a rewrite to remove the virtualness. I think that could be handled in a different PR, though, unless you're feeling adventurous :)
|
Ok, done with my first pass of reviewing, and I am proud. I think I shed a tear somewhere.
I'll take a look at that tomorrow to see if there's a way around it. This is what, the 3rd or 4th time the Filesystem/File thing is causing trouble? |
|
Linking to #5525 which will eliminate the need for templates based on filesystems (since SD will now work with our own FSImpl). |
Still can't merge with head, but structurally sound. Will probably need a completely new PR based off of master/HEAD to do the real PR.
Supercedes esp8266#4912 Refactor the three versions of ESP8266WebServer and *WebServerSecure to a single templated class. Use "using" to enable old, non-templated names to b used (so no user changes required to compile or run). Fixes esp8266#4908 and clean up the code base a lot. Basic tests run (the ones in the example code). No code changes are required in userland except for setting the SSL certificates which now use a cleaner "getServer()" accessor and lets the app use the native BearSSL calls on the WiFiClientSecure object. @devyte should be proud, it removes virtuals and even has template specialization...
|
Closing for the newer #5982 version. |
* Convert ESP8266WebServer* into templatized model Supercedes #4912 Refactor the three versions of ESP8266WebServer and *WebServerSecure to a single templated class. Use "using" to enable old, non-templated names to b used (so no user changes required to compile or run). Fixes #4908 and clean up the code base a lot. Basic tests run (the ones in the example code). No code changes are required in userland except for setting the SSL certificates which now use a cleaner "getServer()" accessor and lets the app use the native BearSSL calls on the WiFiClientSecure object. @devyte should be proud, it removes virtuals and even has template specialization... * Fix HTTPUpdate templates and examples * Fix HTTPUpdateServer library build Need to remove dot-a linkage since there are no .cpp files in the directory anymore due to templates. * Provide backward-compat names for updt template Allow existing code to use the same well known names for HTTPUpdateSecure. * Remove ClientType from all templates, auto-infer Remove the ClientType template parameter from all objects. Simplifies the code and makes it more foolproof. Add a "using" in each server to define the type of connection returned by all servers, which is then used in the above templates automatically. * Can safely include FS.h now that SD/SPIFFS unified * Move the templates/objects to their own namespaces * Fix merge issues with untemplated methods * Address review comments * Fix mock test, remove warnings inside test dir Make the simple mock test CI job pass and clean up any spurious warnings in the test directory. There still are warnings in the libraries and core, but they should be addressed in a separate PR.
Refactor the three versions of ESP8266WebServer and *WebServerSecure to a
single templated class. Use typedefs to enable old, non-templated names to b
used (so no user changes required to compile or run).
Fixes #4908 and clean up the code base a lot.
This is a WIP because to get it to compile I had to move all the F() strings to BSS due to the usual linker segment errors with strings in flash. Need to figure that out before any real commit as OTW we lose maybe 512bytes of heap.
Basic tests run (the ones in the example code) AFAICT.
@devyte should be proud, it removes virtuals and even has template specialization...