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

Extend CsvReader capabilities, move into new library #2805

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Jun 17, 2024

This PR extends the capabilities CsvReader class, introduced in #2403.

Goals:

  • Extend testing, verify efficient use of memory
  • Add Parser capability to allow filtering and processing of (very) large files streamed via network, in files, etc.
  • Add seeking support to allow indexing, bookmarking, etc.
  • Add iterator support

The code has been moved into a separate library.

Changes:

Fix String move to avoid de-allocating buffers

Use longer of two buffers for result. Example:

String s1 = "Greater than SSO buffer length";
String s2;
s1 = ""; // Buffer remains allocated
s2 = std::move(s1); // The move we need to fix

At present this move will result in s1's buffer being de-allocated.
This can lead to performance degratation due to subsequent memory reallocations
where a String is being passed around as a general buffer.

This change uses the larger of the two buffers when deciding how to behave.
Checks added to HostTests to enforce predictable behaviour.

Add CStringArray::release method

Allows efficient conversion to a regular String object for manipulation.

Fix CStringArray operator+=

Must take reference, not copy - inefficient and fails when used with FlashString.

Move CsvReader into separate library

The CsvReader class has been moved out of Core/Data and into CsvReader
which has additional capabilities. Changes to existing code:

  • Add CsvReader to your project's :cpp:envvar:COMPONENT_DEPENDS
  • Change #include <Data/CsvReader> to #include <CSV/Reader.h>
  • Change CsvReader class to :cpp:class:CSV::Reader

TODO:

  • Check documentation
  • Add sample to demonstrate iteration
  • Is there a better way to deprecate current CsvReader.h other than an error? Yes: static_assert.

mikee47 added 2 commits June 17, 2024 07:09
Use longer of two buffers for result. Example:

```
String s1 = "Greater than SSO buffer length";
String s2;
s1 = ""; // Buffer remains allocated
s2 = std::move(s1); // The move we need to fix
```

At present this move will result in s1's buffer being de-allocated.
This can lead to performance degratation due to subsequent memory reallocations
where a String is being passed around as a general buffer.

This change uses the larger of the two buffers when deciding how to behave.
Checks added to HostTests to enforce predictable behaviour.
@slaff slaff added this to the 5.2.0 milestone Jun 17, 2024
@mikee47 mikee47 changed the title [WIP] Extend CsvReader capabilities, new library [WIP] Extend CsvReader capabilities, move into new library Jun 17, 2024
@mikee47 mikee47 changed the title [WIP] Extend CsvReader capabilities, move into new library Extend CsvReader capabilities, move into new library Jun 17, 2024
@slaff slaff merged commit 7020094 into SmingHub:develop Jun 18, 2024
39 checks passed
@mikee47 mikee47 deleted the feature/csvreader-library branch June 18, 2024 08:44
@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