Refactor code - replace DYI structs with STL objects #39
Conversation
|
Thanks! I have several apps with a lot of websocket usage so I will be able to test, but once you will be ready, please make sure the PR is rebased on top of main, and build pass first before testing :-) |
Use STL's platform implementation for mutex locks
replace container with an std::list of unique pointers
use a list of unique_ptrs for _clients container
- LinkedList<AsyncWebRewrite*> _rewrites; + std::list<std::shared_ptr<AsyncWebRewrite> > _rewrites;
repalce LinkedList with std::list<std::unique_ptr<AsyncWebHandler> > _handlers;
|
yeah, I needed your CI check aruduino and 8266 builds, tnx. |
|
@vortigont : i've looked at all the changes: that's pretty cool what you did with the lock class removal also by just applying the mutex for esp32. I love that! Everything is fine for me. I will let you convert the PR back from draft when you'll have finished on your side. I am deploying the PR now in 2 app to test... Note: I won't be able to test the SSE part, but looking at the changes, there's no big risk... |
I've never used that also. Not sure this has any real-life scenarios once WebSockets were introduced. |
|
@vortigont : please see my updated comment: #39 (comment) I think that is fine for the const, I will just indicate that in the release note and move the major digit version since this is still a breaking change. Better do it now than later. Let me know when the PR will be tested on your side and ready to be merged. I already reviewed and tested on my side, all is OK. |
|
Sure! Will add some cosmetic changes by tomorrow |
|
@mathieucarbou |
All these look weird lol': AsyncWebHeader* getHeader(const String& name);
const AsyncWebHeader* getHeader(const String& name) const;
AsyncWebHeader* getHeader(const __FlashStringHelper * data);
const AsyncWebHeader* getHeader(const __FlashStringHelper * data) const;
AsyncWebHeader* getHeader(size_t num);
const AsyncWebHeader* getHeader(size_t num) const; |
ca44b2f to
6f776e2
Compare
and const specifiers for immutable returns make &String arg overloads over const char* + some minor cleanups
|
I think that's it. Enough for one PR :) |
|
@vortigont : FYI I had quite a few issues when trying to include the release in some other projects. Most of them being linked that So I added an equivalent overload for ESP32. This still breaks the API, but at least users can just remove the But this caused me hours of headaches because I add to update / release more than 10 projects / libraries. If we have some complains, we will need to put back That's the only issue I saw. Also, I added to library.json (just in main, not released yet) I updated the CI to add this |
|
Great, thanks! I also want to refactor all those static char strings with mime-types, etc... into separate units for ESP32 and 8266 to get rid of those |
|
@vortigont : FYI have opened #46 to put back these methods but with a deprecation notice. Would you be able to have a look quickly before I merge ? Thanks! |
|
Sure, let me check it and get back! |
|
@vortigont : I ran into a stack trace today, linked to the object destructions: Which led me to this line: https://github.com/mathieucarbou/ESPAsyncWebServer/blob/main/src/AsyncJson.h#L123 Not sure it is the culprit, but what do you think about removing it ? There is no destructor nowhere in the super classes or in the sub classes anymore. I don't know why only this class would have a virtual destructor and not the other ones. And Note: the stack trace above iis really hard to reproduce - I need to stress the web server a bit directly at startup. |
|
that's pretty weird. There only String I see in |
|
@vortigont : FYI reminder for next time ;-) |
|
Ugh... people are still using 8266's code on esp32, need to push them somehow anyway, bit by bit :) |
|
No it's not 8266 code: as I explained in the fix commit, this is to use the esp32 embedded binary feature of pio.
There is nothing related to esp8266. The _P method was used because there was no variant without _P supporting uint8_t arrays.
|
|
An other second one is the char* equivalent of the String one... So a mere overload like we have elsewhere. So except the macro use for const char*, there is nothing related to 8266. |
|
oh, yeah, now I got it. So some other's code were using |
obsolete all local
LinkedListcontainers, replaced with std::listreplace self-implemented mutex structures with std:mutex
@mathieucarbou take you time to get through this
This is WIP, I'm currently testing it, but I do not have any heavy-running setups, so any feedback would be appreciated