Skip to content
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

Improve FlashString portability #2796

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Jun 10, 2024

This PR removes copy support from the FlashString library. Doing so simplifies the code, improves performance and portability to other compilers (e.g. clang).

The FlashString library previously supported copies (references) like this::

 FlashString emptyString;
 FlashString stringCopy(FS("Inline string"));

 DEFINE_FSTR_DATA_LOCAL(flashHelloData, "Hello");
 auto myCopy = flashHelloData;

These will now fail to compile.
Copy construction and assignment has been explicitly deleted so avoid unintentional side-effects.
Objects should always be passed by reference.

This change has the additional benefit of catching pass-by-copy errors. These function/method templates have been fixed:

  • fileSetContent
  • IFS::File::setAttribute
  • IFS::FileSystem::setUserAttribute
  • IFS::ArchiveStream::setUserAttribute

@mikee47 mikee47 force-pushed the feature/flashstring-portability branch 2 times, most recently from 8f7d9e9 to 6bcab31 Compare June 10, 2024 21:59
mikee47 added 4 commits June 11, 2024 06:55
The ability to copy FlashString objects does not really have any concrete benefit,
other than avoiding the need to patch ArduinoJson.

This change simplifies code, removes the need for casting and improves portability to other compilers.
@mikee47 mikee47 force-pushed the feature/flashstring-portability branch from 6bcab31 to 9c5ee6d Compare June 11, 2024 05:55
@slaff slaff added this to the 5.2.0 milestone Jun 11, 2024
@slaff
Copy link
Contributor

slaff commented Jun 11, 2024

@mikee47 can you add this information also to the UPGRADING document?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 11, 2024

@mikee47 can you add this information also to the UPGRADING document?

? Already there...

@slaff slaff merged commit cf4e1c9 into SmingHub:develop Jun 11, 2024
38 checks passed
@mikee47 mikee47 deleted the feature/flashstring-portability branch June 11, 2024 09:19
@slaff slaff mentioned this pull request Jul 4, 2024
5 tasks
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.

2 participants