Skip to content

Remove usage of Arduino constant emptyString and use our own instead#417

Draft
mathieucarbou wants to merge 1 commit intomainfrom
emptyString
Draft

Remove usage of Arduino constant emptyString and use our own instead#417
mathieucarbou wants to merge 1 commit intomainfrom
emptyString

Conversation

@mathieucarbou
Copy link
Copy Markdown
Member

@mathieucarbou mathieucarbou commented Apr 3, 2026

This PR is a cleanup in order to use our own empty string constant instead of Arduino one.

This will also help support HOST as done in PR #416

@mathieucarbou
Copy link
Copy Markdown
Member Author

@MitchBradley FYI

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces uses of the Arduino emptyString constant with a project-owned asyncsrv::empty constant.

Changes:

  • Updated default arguments across response/request APIs to use asyncsrv::empty.
  • Replaced emptyString assignments/returns throughout request parsing and authentication helpers.
  • Updated examples to return/assign asyncsrv::empty instead of emptyString.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/WebResponseImpl.h Switches AsyncBasicResponse String overload default arg to asyncsrv::empty.
src/WebRequest.cpp Replaces emptyString in parsing/temp fields and several const String& return paths.
src/WebHandlers.cpp Updates AsyncFileResponse content-type default argument usage.
src/WebAuthentication.cpp Uses asyncsrv::empty for early-return empty String values on errors.
src/ESPAsyncWebServer.h Updates multiple overload defaults and const String& accessors to use asyncsrv::empty.
src/AsyncEventSource.cpp Returns asyncsrv::empty on allocation failure.
examples/Templates/Templates.ino Updates template processor “no value” return to asyncsrv::empty.
examples/RequestContinuation/RequestContinuation.ino Clears message using asyncsrv::empty.
examples/ChunkRetryResponse/ChunkRetryResponse.ino Clears trigger string using asyncsrv::empty.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MitchBradley
Copy link
Copy Markdown

@mathieucarbou Thanks for addressing this. After it is merged, should I close #416 and submit a revised version, after addressing copilot's concerns?

@mathieucarbou-ibm
Copy link
Copy Markdown

@mathieucarbou Thanks for addressing this. After it is merged, should I close #416 and submit a revised version, after addressing copilot's concerns?

You just need to rebase your HOST PR on top of branch emptyString instead of main.

Also please do not forget to open a separate PR for the state machine refactoring. This one is more requires more review and testing.

Thank you very much for these !

@MitchBradley
Copy link
Copy Markdown

One thing that occurred to me is that instead of #if defined(ESP32) || defined(HOST}, it might be better to create a new macro with a name like MULTICORE, so one could write #ifdef MULTICORE and put the "ESP32 || HOST" logic in one place.

@MitchBradley
Copy link
Copy Markdown

Or perhaps a conditionally compiled LOCK macro so one could write

LOCK(_client_queue_lock)

@mathieucarbou
Copy link
Copy Markdown
Member Author

One thing that occurred to me is that instead of #if defined(ESP32) || defined(HOST}, it might be better to create a new macro with a name like MULTICORE, so one could write #ifdef MULTICORE and put the "ESP32 || HOST" logic in one place.

I am not sure that the content of the conditions only apply to multicore.
They apply to ESP32 and libretiny at the moment because both are really close, compared to 8266 and RPI.

Right now I would leave as it is with the 3 conditions... They are at the moment more linked to the common things provided by these platforms than being multicore or not I think.

@mathieucarbou
Copy link
Copy Markdown
Member Author

Or perhaps a conditionally compiled LOCK macro so one could write

LOCK(_client_queue_lock)

That's the equivalent of what @willmmiles put in place in AsyncTCP.
But please don't fire in every direction right now.

I would just focus at the Linux/Mac support with HOST macro, then you can open a new PR with the state machine.

And if you are willing to improve how the locking is down using macros, please open another PR.

As long as you keep the work in single isolated unit of work commits and PRs.

Thanks 👍

namespace asyncsrv {

static constexpr const char empty[] = "";
inline const String emptyString{};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have no objections to formally including this in our namespace, but we might consider still borrowing the Arduino one if it's there to reduce code bloat.

#ifdef ARDUINO
using ::emptyString;
#else
inline const String emptyString{};
#endif

In my quick build test of PerfTest on arduino-3 this resulting in saving ~700 bytes of code space and 24 bytes of SRAM -- indicating that the inline const definition was still being placed in SRAM (likely because the destructor isn't constexpr-safe, even though it'll never get called).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks !

Do you think the increase cost is because of inlining ?

Arduino just defines it as :

const String emptyString;

I think this can even be better for places where a ref is returned ?

Copy link
Copy Markdown
Member Author

@mathieucarbou mathieucarbou Apr 3, 2026

Choose a reason for hiding this comment

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

I am tempted maybe to do:

#ifdef ARDUINO
using ::emptyString;
#else
static const String emptyString;
#endif

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is ARDUINO the right #define to switch on? I'm not sure that distinguishes between Arduino frameworks that have emptyString and those that do not. For reference, framework-arduinoespressif32 has extern const String emptyString in WString.h and const String emptyString; in WString.cpp, while framework-arduinopico (which, unlike ESP, uses ArduinoAPI) has extern const String emptyString; in Arduino.h and const String emptyString = ""; in main.cpp, with WString.h in api/deprecated/ and there is no WString.cpp. Libretiny has the const String formulation in wiring_compat.*. Arduino-Emulator does not have it at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That’s also true.
And in that case would probably weight in favor of the static const String version that is not inlined ?

Copy link
Copy Markdown

@willmmiles willmmiles Apr 4, 2026

Choose a reason for hiding this comment

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

The increase in cost is due to declaring a unique new object - it has to live somewhere in memory. inline's only purpose here is to inform the C++ compiler that it's OK to coalesce the objects across translation units. A non-inline version will instantiate a unique string in RAM for every translation unit that uses it -- it'll only make it worse. (The extern const version with a static declaration in one translation unit is the pre-C++17 best practice to declare a single instance of a global shared object.)

Re define selection: yes, there's undoubtedly room to improve the platform feature detection. I'd assumed that this symbol was part of the Arduino standard, though it appears instead it is merely a common extension. (Perhaps Arduino-Emulator is doing users a disservice by omitting common but non-standard APIs...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@willmmiles : I have applied your suggestion. I will let do a final review and we merge ?

@mathieucarbou mathieucarbou marked this pull request as draft April 3, 2026 21:29
@mathieucarbou mathieucarbou force-pushed the emptyString branch 2 times, most recently from 6754cab to 41a0e2f Compare April 4, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants